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

atlas tries to checkout incorrect commit when a requires: dep doesn't specify a commit #18753

Closed
timotheecour opened this issue Aug 26, 2021 · 1 comment

Comments

@timotheecour
Copy link
Member

timotheecour commented Aug 26, 2021

after trying to fix other atlas issues (#18750, #18751) i'm running into the following issue:

Example

after fixing #18750, #18751, running this:

bin/nim c -r tools/atlas/atlas.nim clone https://github.com/disruptek/balls

Current Output

after instrumenting code to produce an assert instead of erroring with zero context (quit(1) is bad as it doesn't any context on errors; at least there should be a debug flag to allow giving errors, possibly at the nim-level so it applies to all programs calling quit):

/Users/timothee/git_clone/nim/Nim_prs/tools/atlas/atlas.nim(498) atlas
/Users/timothee/git_clone/nim/Nim_prs/tools/atlas/atlas.nim(479) main
/Users/timothee/git_clone/nim/Nim_prs/tools/atlas/atlas.nim(380) clone
/Users/timothee/git_clone/nim/Nim_prs/tools/atlas/atlas.nim(284) checkoutCommit
/Users/timothee/git_clone/nim/Nim_prs/tools/atlas/atlas.nim(168) warn
/Users/timothee/git_clone/nim/Nim_prs/tools/atlas/atlas.nim(164) message
/Users/timothee/git_clone/nim/Nim_prs/lib/system/assertions.nim(38) failedAssertImpl
/Users/timothee/git_clone/nim/Nim_prs/lib/system/assertions.nim(28) raiseAssert
/Users/timothee/git_clone/nim/Nim_prs/lib/system/fatal.nim(53) sysFatal

it also shows this:
[Warning] (nim-bytes2human) package has no tagged releases;

and after instrumentation:

 (name: ..., url: "https://github.com/juancarlospaco/nim-bytes2human", commit: "<invalid commit>", rel: strictlyLess);

the problem is that https://github.com/disruptek/balls recursively depends on https://github.com/juancarlospaco/nim-bytes2human which doesn't have a tagged release;

note that https://github.com/disruptek/testes/blob/d9db2ad09aa38fc26625341e1b666602959e144f/testes.nimble (which is checked out at that revision by atlas) contains:

requires "https://github.com/juancarlospaco/nim-bytes2human"

Expected Output

it should work because nim-bytes2human doesn't contain a specific revision, so should checkout HEAD instead of attempting to checkout a (nonexistent) tag

Either that or we should clarify somewhere that requires "https://github.com/juancarlospaco/nim-bytes2human" is invalid, or that a tag must be specified;

in either case, better error reporting and diagnostic is needed here; even if https://github.com/juancarlospaco/nim-bytes2human gets fixed (which is easy and likely to happen), the same problem will occur in other instances.

Additional Information

1.5.1 98f7254

the culprit is this code:

      if tokens.len == 1:
        # nimx uses dependencies like 'requires "sdl2"'.
        # Via this hack we map them to the first tagged release.
        # (See the `isStrictlySmallerThan` logic.)
        tokens.add "<"
        tokens.add InvalidCommit
@Araq
Copy link
Member

Araq commented Aug 30, 2021

... so should checkout HEAD instead of attempting to checkout a (nonexistent) tag ...

No, checking out HEAD is completely against the "pick minimum version" and "reproducible builds" design. You're probably right and there are real bugs and issues in Atlas and it's unusable for you in its current state. That's ok, so don't use it and cut down the noise. There are much more important things to do, like nim-lang/RFCs#416

@Araq Araq closed this as completed Aug 30, 2021
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 a pull request may close this issue.

2 participants