-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fixes #25235; skipParentCfg in nim.cfg
#25307
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
base: devel
Are you sure you want to change the base?
Conversation
| let prevSkip = optSkipParentConfigFiles in conf.globalOptions | ||
| conf.skipParentDetectionMode = true | ||
| try: | ||
| result = readConfigFile(cfgPath, cache, conf) and |
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.
What happens to --skipParentCfg in .nims files?
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.
No change to --skipParentCfg in .nims files after this PR. That is going to require significantly more invasive changes and it's not worth it. configEnablesSkipParent should only be applied to nim.cfg files as this happens after the system configs have been loaded in and before the final compilation target specific one.
Now that you mention it though, I think this is wrong because module specific configs have to be checked too:
Line 299 in 3f75c15
| if optSkipParentConfigFiles notin conf.globalOptions and not configEnablesSkipParent(conf, cache, pd / cfg): |
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.
Okay, if skipParentCfg is only necessary for cfg files. Perhaps a warning or something is needed if skipParentCfg is used in the .nims files
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.
not a bad idea, but I don't really care about that. I believe you can push to this branch if you want since you are a maintainer.
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.
hmm .. the way nimble works, it puts a config section in config.nims - not having nims support would mean it also has to modify nim.cfg - in other words, having support for .cfg files is a good step forwards but it also makes it harder to teach and use the feature, so longer-term, .nims support is also important.
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 can agree with the rationale, but the way I see it this feature solves a very specific problem. I think its relatively common to run into seeing as how both of us did, but it's still more of a "bail out" feature. Why would nimble need to use this? It's hard to imagine why this feature would be used for general purposes. The way I see it, this feature depends on how projects are nested on a specific machine, or in some cases how different entry points are nested in self contained repos (meaning this is required in a subdir of the AIO structure not the root). Self contained projects should not worry about the host machine's configuration hierarchy. It would be an impolite move to put skipParentCfg at the root of a code repo, for example.
Also, as a side note, Nim 3 is posturing to have 4 different config files with separate features on top of supporting these two legacy ones so apparently we are moving in this direction.
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 .nims files
in nims files, for ease of processing, this could also be a {.pragma.} which falls outside of the usual side-effecty processing.
Why would nimble need to use this
Nimble controls how workspaces are created, for the user - when creating the workspace, we want nimble test inside a "nested" library to work independently of the parent project configuration - therefore, when nimble injects a package into a nested workspace, it would also "stop" parent cfg configuration for that library (and adjust --path options accordingly) as part of its "install the package" setup - ie this is not something a library developer would do - instead, the package manager would be responsible for it and the skipParentCfg is the means of getting it done.
The mechanism nimble uses for this is to patch config.nims with a special section (atlas also does this) - it's not the prettiest of approaches, but it has the winner property that it's backwards-compatible with old nim releases.
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.
Alright then. I don't use nimble or atlas so that might help clear it up for other people like me. When you say it "injects" the packages I assume this is some weird stuff that isn't solved by making a nimble template that drops the nim.cfg by default or just having one set up in the "tests" directory.
| cfgPath: AbsoluteFile): bool = | ||
| let prevMode = conf.skipParentDetectionMode | ||
| let prevSkip = optSkipParentConfigFiles in conf.globalOptions | ||
| conf.skipParentDetectionMode = true |
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.
It seems that the order of lexer error message is changed, not sure whether that's a side effect or not. Probably, a custom empty errorHandler can be used for lexer
| runNimScriptIfExists(pd / DefaultConfigNims) | ||
|
|
||
| if conf.projectName.len != 0: | ||
| # new project wide config file: |
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.
this comment can be kept under if hasProjectCfg:
I'm more than happy to remove/change Nim 3's config system in favor of something better. There have been fruitful discussions about how the compiler should have a notion of a "package" and how a configuration should be package-wide. We can also deprecate |
fixes #25235
in
commands.nimandoptions.nimskipParentDetectionModethat determines if we are checking forskipParentCfgskipParentDetectionModeinprocessSwitchto ignore all cfg entries exceptskipParentCfgin
loadConfigs:skipParentCfgskipParentDetectionModeand disables side effects inparseDirectivewhile in this modeskipParentCfgis found, so we are left with a list of cfgs to process