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

Substitute $nimbleDir in --path flags #12750

Merged
merged 5 commits into from
Nov 28, 2019
Merged

Substitute $nimbleDir in --path flags #12750

merged 5 commits into from
Nov 28, 2019

Conversation

genotrance
Copy link
Contributor

Per discussions in https://gist.github.com/genotrance/35504ed532f9cb836f11fd02b5bf1145.

Adding Nim capability to handle $nimbleDir substitution in --path flags

  • $nimblePath and $nimbleDir both are supported
  • Handled differently than other $ substitutions since it is one to many
  • Every $nimbleDir added to Nim will be added to searchPaths
    • If $nimbleDir = ["$home/.nimble/pkgs", "$projectpath/nimbledeps/pkgs"]
    • --path:"$nimbleDir/pkg-0.1.0" will searchPaths.add(["$projectpath/nimbledeps/pkgs/pkg-0.1.0", "$home/.nimble/pkgs/pkg-0.1.0"])

Cc @Araq @dom96 @disruptek

@genotrance genotrance changed the title Nimbledir Substitute $nimbleDir in --path flags Nov 27, 2019
Copy link
Contributor

@disruptek disruptek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the paths should be added to lazyPaths, since they reflect automated lazy additions. Otherwise, we cannot tell the difference between those specified via --nimblePath or those specified via --path. This is important when trying to make sense of the nim.cfg.

compiler/nimblecmd.nim Outdated Show resolved Hide resolved
result.add p
else:
result.add p

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not simply generalize pathSubs to be an iterator that can yield 1+ values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would cause too much impact - pathSubs is used in other places too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that's the idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pathSubs() is also used by the patchFile() nimscript proc as well as getModuleName() calls. I don't think either benefits from a $nimbleDir substitution, Araq might know better. Even if we change pathSubs() to return a seq[AbsolutePath] and pick [0], it will propagate all over the place since processPath() would also need to return a seq and is called in many places which are irrelevant for $nimbleDir.

Alternative is to simply substitute $nimbleDir with the last nimbleDir added to Nim in pathSubs() and call it a day but the current implementation is marginally better functionality without adding too much code with no benefit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not about what benefits from it; we cannot know the use of this code, so it should be made as consistent as possible. If it "propogates all over the place", so much the better; that is in fact what we want!

A pathSubs iterator won't clash with a proc, but you can rename it if you want -- I just want a way to automatically use a $nimbledir substitution when one exists, as I currently do for pathSubs.

This is not "no benefit"; it's solving a real need for the tools that use these substitutions.

config/nim.cfg Show resolved Hide resolved
@genotrance
Copy link
Contributor Author

I think the paths should be added to lazyPaths, since they reflect automated lazy additions. Otherwise, we cannot tell the difference between those specified via --nimblePath or those specified via --path. This is important when trying to make sense of the nim.cfg.

These paths are being specified using --path:"$nimbleDir/xyz" hence searchPaths.

Copy link
Contributor

@disruptek disruptek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I'm responding to. I don't think this makes any sense when lazyPaths directly matches the semantics we want.

@@ -587,6 +588,17 @@ proc pathSubs*(conf: ConfigRef; p, config: string): string =
if "~/" in result:
result = result.replace("~/", home & '/')

proc nimbleSubs*(conf: ConfigRef; p: string): seq[string] =
let pl = p.toLowerAscii
if "$nimblepath" in pl or "$nimbledir" in pl:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've noted that --noNimblePath may be misleading in your RFC, I think that making $nimblepath an alias for $nimbledir is problematic for that same reason. No reason to support both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put both because Nimble calls this nimbleDir whereas Nim calls it nimblePath in its command line help as well as in the code.

Further, there's $projectdir and $projectpath in pathSubs() which are synonyms so I added the same for $nimble* as well.

Comment on lines +132 to +135
let i = conf.nimblePaths.find(path)
if i != -1:
conf.nimblePaths.delete(i)
conf.nimblePaths.insert(path, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you remove addNimblePath and implement a prependNimblePath?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addNimblePath() adds paths discovered into lazyPaths. This new code simply saves the base $nimbleDir for later usage.

Comment on lines 599 to 600
else:
result.add p
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: move this at the start of the function to reduce nesting:

if "$nimbledir" notin pl: return @[p]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return doesn't work anymore in an iterator. Tried {.closure.} but then nothing worked. Skipping this for now.

compiler/options.nim Outdated Show resolved Hide resolved
tests/nimble/tnimblepathdollar.nims Show resolved Hide resolved
Copy link
Contributor

@disruptek disruptek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improved, but this whole PR no longer makes any sense to me based on what I understand of your goals.

Why are you using --nimblePath at all? You can simply --noNimblePath followed by numerous --path statements to accomplish your mission of locking down each and every requirement. Then this patch could simply add a record of lazyPaths added via --nimblePath, which would actually be useful.

@genotrance
Copy link
Contributor Author

Why are you using --nimblePath at all? You can simply --noNimblePath followed by numerous --path statements to accomplish your mission of locking down each and every requirement.

We could but then the nim.cfg will be useless to check into source control because the absolute paths will be host specific. If a project has an existing nim.cfg, it could get inadvertently checked in in this state.

The idea is to use a specific version of a package. It is expected to be in a Nimble directory hence expanding $nimbleDir into searchPaths is reasonable. It is not a lazy search where any version of a package can be loaded.

Implementing $nimbleDir substitution as is in this PR gives --path a new capability and doesn't break any existing use cases. Nim is already aware of Nimble paths and we are simply using them in a substitution to make it portable.

@Araq Araq merged commit 010067f into nim-lang:devel Nov 28, 2019
@disruptek
Copy link
Contributor

We already discussed this and came to the conclusion that nim.cfg would be used for site-specific configuration and package managers could blow away any paths found therein. So, any paths in nim.cfg have already been deemed "useless". Indeed, the fact that any one of your --path substitutions can be corrupted by changes to the substitutions themselves should make this plain.

nim.cfg can be used for other purposes than path specification, and this is why it should be checked into source control.

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.

4 participants