Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated fix for #280 with dependencies #464

Closed
wants to merge 7 commits into from
Closed

Updated fix for #280 with dependencies #464

wants to merge 7 commits into from

Conversation

genotrance
Copy link
Contributor

Original fix ran "before install" before dependencies were installed.

@genotrance
Copy link
Contributor Author

I don't see why #280 got linked to these unrelated comments. Regardless, this updated fix has a new issue - if you run nimble install on a package in a local directory, it runs before install twice since the execHook() runs in doAction() as well as inside install().

I'm thinking we need to keep track of whether before and after hooks have run already or not. I think this can be done by changing PackageInfo.preHook and postHook into Table[string, bool] but it might be overkill. Suggestions appreciated.

@genotrance
Copy link
Contributor Author

I've fixed this by moving the pre and post execHook() calls within installFromDir(). This ensures that it works correctly whether installing locally or after download.

Additionally, I prevent the generic execHook() calls in doAction() from running for actionInstall, similar to actionCustom since they are handled elsewhere.

@genotrance
Copy link
Contributor Author

Build failed in test for issue #428 - not sure how it is related to this change.

@dom96
Copy link
Collaborator

dom96 commented Jun 20, 2018

Relevant issue: https://forum.nim-lang.org/t/3949 (which we should also fix)

@dom96
Copy link
Collaborator

dom96 commented Sep 1, 2018

Closing in favour of #482.

@dom96 dom96 closed this Sep 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants