-
Notifications
You must be signed in to change notification settings - Fork 188
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
Allow locking nim version in nimble.lock #1017
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fair. Though can you elaborate what will end up in the lock file when nimble lock
is called?
src/nimblepkg/packageparser.nim
Outdated
if not pkgInfo.basicInfo.name.isNim: | ||
if dir.len == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: instead of adding another nesting to this if, please just put the condition into one of the existing if statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I will have to add one if in the else clause as well. It will be:
if not foo and bar:
...
elif not foo
...
To me this looks less "clean. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, in that case if you wouldn't mind refactoring it out into its own proc that would be the best of both worlds :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the whole if dir.len == 0:
if statement (its body and the else
) into a validatePackageStructure
proc, then only call it under the if not pkgInfo.basicInfo.name.isNim:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will do.
src/nimblepkg/packageinfo.nim
Outdated
@@ -530,6 +528,9 @@ proc hash*(x: PackageInfo): Hash = | |||
proc getNameAndVersion*(pkgInfo: PackageInfo): string = | |||
&"{pkgInfo.basicInfo.name}@{pkgInfo.basicInfo.version}" | |||
|
|||
proc isNim*(name: string): bool = | |||
result = name == "nim" or name == "nimrod" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nim hasn't been known by that name in long enough to exclude it IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will do. I will rename a few functions using nimrod in their name as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nim hasn't been known by that name in long enough to exclude it IMO
I reverted that part - nimrod is used in the tests so I don't think that this PR is the place to go and fix them.
src/nimblepkg/tools.nim
Outdated
proc getNimrodVersion*(options: Options): Version = | ||
let vOutput = doCmdEx(getNimBin(options).quoteShell & " -v").output | ||
let vOutput = doCmdEx(getNimBin(options) & " -v").output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you're gonna do that then rename getNimBin
to getNimBinQuoted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively just revert this change, quoteShell is much better closest to doCmdExe to ensure we use it (plus other programs like choosenim might be using getNimBin and won't be expecting quoteShell)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will revert this refactoring and keep the quoteShell
in the caller.
Here is what it looks like(I have patched packages.json locally to make that work until we have an installable version in main repo)
|
@dom96 please disregard this PR - refer to nim-lang/Nim#20179 (comment) I have to revisit the implementation. |
Suggested a solution :) |
cf302e2
to
045134d
Compare
@dom96 applied your comments(hopefully I haven't missed anything). |
3f8d087
to
b4b2ace
Compare
72cd1c3
to
11f85b7
Compare
why is |
I don't have a strong preference for that. The idea was to avoid managing nim by accident and having it more explicit. Zah suggestion was to do that with flag, but I did it with |
* Rename compiler to nim - see nim-lang/nimble#1017 and nim-lang/Nim#20179 * Better description of the Nim package Co-authored-by: zah <[email protected]>
As per my discussion with @arnetheduck I was trying to add flag to make locking possible with @arnetheduck I think that this feature should be held on till we tag nim version, WDYT? |
I believe this is the case for any nimble dependency, ie nimble will happily download any random version of anything, depending on what is in the repo at the time and what the user happens to have in their global cache. I think the two issues can be addressed separately - in particular, I imagine that if an application wants a specific nim version, it would indicate so in its |
8c4c6e3
to
28ca4ea
Compare
8041276
to
5f4813a
Compare
Depends on nim-lang/Nim#21313 and nim-lang/Nim#21314 |
This caused the lockfiles to ignore certain dependencies
Removes previous hack that needed to read current nimble file state This changes the lock file structure, but compaitability shouldn't be affected
Using nimlangserver since that was that project I first noticed it with
Will now investigate other tests
Force yes in tests Fix tests Go back to non destructive uninstall for deps
Error says it is due to stdlib but it works a few lines later
- Fixes nim-lang#953 Allow having nim as locked dependency. - I will add unit tests once we agree on the approach and once nimble related changes in nim are merged (I will link the PR in comment). Ditto for the documentation. In order that change to work we have to add nim package in nimble packages repo and also add alias compiler -> nim to avoid breaking backward compatibility. Here it is the flow: ``` bash nimble develop nim nimble lock ``` After that `nimble install` and `nimble build` commands will use the locked `nim` version
@Araq It is good now. |
nimble install
doesn't respect newly downloadedpackages.json
#953Allow having nim as locked dependency.
changes in nim are merged (I will link the PR in comment). Ditto for the
documentation.
Here it is the flow:
After that
nimble install
andnimble build
commands will use the lockednim
version