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

fix lots of issues with atlas #18756

Closed
wants to merge 7 commits into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Aug 27, 2021

after this PR

this now works instead of failing (with several unrelated errors)

bin/nim r tools/atlas/atlas.nim --atlasdir:/tmp/d06e/atlas --outfilecfg:/tmp/d06e/my.nim.cfg clone https://github.com/disruptek/balls

note, the --path generated are correct wrt --atlasDir and --outFileCfg

future work

  • add support for NIM_ATLAS_DIR, analog to NIMBLE_DIR
  • atlas should eventually be able to install a dependencies as part of bootstrapping process (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?

############# begin Atlas config section ##########
--noNimblePath
--path:"../balls"
--path:"../grok"
...
############# end Atlas config section   ##########

what's the exact intention behind --noNimblePath, and can we use nim-lang/RFCs#291 instead, which would be far more flexible?

@timotheecour timotheecour marked this pull request as ready for review August 27, 2021 05:33
@timotheecour timotheecour changed the title fix some issues with atlas fix lots of issues with atlas Aug 27, 2021
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Aug 27, 2021
@@ -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
Copy link
Member

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.

Copy link
Member Author

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
Copy link
Member

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.

Copy link
Member Author

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)

Copy link
Member Author

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.

Copy link
Member

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.

@Araq
Copy link
Member

Araq commented Aug 29, 2021

It's unclear what this PR does and it needs to be redone.

  1. Read the documentation.
  2. Fix real bugs.
  3. or - do a real refactoring adding some value that don't confuse bugs/assertions with decent error handling.

@Araq Araq closed this Aug 29, 2021
@timotheecour
Copy link
Member Author

timotheecour commented Aug 29, 2021

Read the documentation.

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:

  • P1 how it'd help with stdlib/compiler/tools depend on a 3rd party package; in particular those need reproducible builds so locking down hashes of transitive dependencies is need
  • P2 when to use nimble vs atlas
  • P3 how will it work on projects that already depend on nimble, given that it adds --noNimblePath; in particular, how to ensure the (transitive) dependencies (say pkg/regex) remain local and don't interfere with user code that also happens to depend on pkg/regex (possibly with a different, incompatible version)

P3

Link to relevant discussion: https://discord.com/channels/371759389889003530/768367394547957761/880738592056565790 where i suggested --moduleoverride (#18496) as a simple (IMO elegant) solution to P3, which would allow this:

# atlas adds this to $nim/config/nim.cfg: --moduleoverride:pkg:../lib/std/atlas:std

which would cause import pkg/regex (and the code inside it referring to transitive packages, eg import pkg/unicodedb) to resolve to import $nim/lib/std/atlas/regex, for all modules within a specified prefix (eg std to make this apply to stdlib modules).

In particular, this wouldn't need --noNimblePath, it wouldn't expose the privately imported package to client code, and wouldn't interfere with client code that uses pkg/regex (possibly different version)

PMs (Nimble included) should be able to import parse_requires.nim for a common ground.

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+

@Araq
Copy link
Member

Araq commented Aug 30, 2021

It's a tool for myself and doesn't need an RFC, much like your nim_dbg didn't need an RFC. We've clarified in the linked discussion that it should not be used for introducing deps into the compiler.

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

I don't agree and I don't want to spend time on discussing it anymore.

@timotheecour
Copy link
Member Author

well you wrote here: nim-lang/RFCs#411 (comment)

So provide a Nimble package as the foundation for these other Nimble packages and use "atlas" to be able to use it in Nim's core. I wrote Atlas for a reason.

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.

@Araq
Copy link
Member

Araq commented Aug 31, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round
Projects
None yet
2 participants