-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix lots of issues with atlas #18756
Conversation
1f6dbc0
to
c48614d
Compare
c48614d
to
ec2df7b
Compare
@@ -24,6 +36,8 @@ Command: | |||
search keyw keywB... search for package that contains the given keywords | |||
|
|||
Options: | |||
--outFileCfg:file (required) write cfg to `file` | |||
--atlasDir:dir (required) root of atlas packages |
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.
why are these required now? They should default to the cwd.
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.
see linked issues ...
if c.workspace.len == 0: | ||
error "--atlasDir:dir must be provided" # refs #18750 | ||
if c.outfilecfg.len == 0: | ||
error "--outFileCfg:file must be provided" # refs #18751 |
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, make it work according to its docs, which you apparently never read but then do so.
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.
indeed, i did miss tools/atlas/atlas.md ; it's easy to miss though since it's not well referenced,
i had searched docs in a few places (including https://nim-lang.github.io/Nim/tools.html, and just assumed all there was the small comment in tools/atlas/atlas.nim)
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.
even after reading the docs I find the pre-existing behavior (via a workspace) to be the wrong design, as it doesn't work well with existing user setups (like the 1 i had which triggered #18750)
it's likely others will have the same problem; the workspace, if hard-coded, should be contained within the source (in-tree, not out-of-tree), that way you don't mess with external code and still allow multiple nim repos (or nimble package clones) to peacefully co-exist in same dir without interference.
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.
Make it work as I documented it please, it works well for me and other use cases can be supported via command line overrides.
It's unclear what this PR does and it needs to be redone.
|
Where was the RFC for atlas? Please write one so the design decisions can be discussed properly instead of baked in as of the PR that introduced it. I'm not necessarily questioning the utility of the tool (depending on the direction it's headed), I'm questioning the specific design decision such as writing out-of-tree which breaks existing use cases, overwrites unrelated user files and goes against best practices. Which projects use atlas? AFAIK this was the 1st attempt at using it, it doesn't look promising when the issues and PR I've opened are being closed (eg #18751 (comment), #18754 (comment), etc), the problems are still there for anyone brave enough to try to use it. The RFC should in particular cover concrete use cases:
P3Link to relevant discussion: https://discord.com/channels/371759389889003530/768367394547957761/880738592056565790 where i suggested # atlas adds this to $nim/config/nim.cfg: --moduleoverride:pkg:../lib/std/atlas:std which would cause In particular, this wouldn't need
this was advertised as a major point, except that atlas doesn't understand most of the requires syntax (eg <= and >= are confusd) and is incompatible with how nimble does it, see issues I've filed (and the closed ones), refs https://github.com/nim-lang/Nim/issues?q=label%3Aatlas+ |
It's a tool for myself and doesn't need an RFC, much like your
I don't agree and I don't want to spend time on discussing it anymore. |
well you wrote here: nim-lang/RFCs#411 (comment)
which is in direct contradiction I wouldn't have wasted time if I had known it's a tool for yourself not suitable for adding dependencies to compiler/stdlib. |
Well found that out after the discussions, keep the order of events in mind. |
requires: dep
doesn't specify a commit #18753--atlasdir:dir
specifying root of atlas dirs; the previous hard-coded logic was flawed (refs atlas overwrites user files #18750)--outfilecfg:file
after this PR
this now works instead of failing (with several unrelated errors)
note, the --path generated are correct wrt --atlasDir and --outFileCfg
future work
NIM_ATLAS_DIR
, analog toNIMBLE_DIR
sh build_all.sh
), not just for tools/compiler but also for stdlib; this is the only way to avoid endless discussions regarding whether something should belong to stdlib or nimble package (while allowing stdlib modules to depend on such modules); towards this end, we should make atlas install a dummy dependency as part of bootstrap to ensure this process works and keeps working (eg a lib/std/private/usesatlas.nim can be added which imports some atlas-installed package); once this is stable, actual dependencies can be used.questions for reviewer
outfilecfg
will contain--noNimblePath
; how can this work? won't that prevent users from using nimble installed packages?what's the exact intention behind
--noNimblePath
, and can we use nim-lang/RFCs#291 instead, which would be far more flexible?