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

Add warning message when cloning the HEAD. #809

Merged
merged 3 commits into from
Jun 27, 2021

Conversation

EchoPouet
Copy link
Contributor

In answer of this issue, it is important to indicate that the developer use a package without git release tag.

This message, I hope, can also generate more serious in development process and improve the quality. In the common world, a release version has a git tag.

Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Thanks for this but it's hard to know when this message will be shown. For example, it would be annoying to see it when the user explicitly asks for #head.

@EchoPouet
Copy link
Contributor Author

The warning message is displayed only if the repo hasn't tags when a version is needed. I made some tests:

zip package hasn't tags:

./src/nimble install zip@#head
Downloading https://github.com/nim-lang/zip using git
  Verifying dependencies for zip@#head
 Installing zip@#head
   Success: zip installed successfully.

./src/nimble install zip
Downloading https://github.com/nim-lang/zip using git
   Warning: No corresponding tag found with required version. We grab HEAD.
  Verifying dependencies for [email protected]
 Installing [email protected]
    Prompt: [email protected] already exists. Overwrite? [y/N]
    Answer: y
   Success: zip installed successfully.

karax package has tags:

nimble install karax@#head
Downloading https://github.com/pragmagic/karax using git
   Warning: Package 'karax' has an incorrect structure. It should contain a single directory hierarchy for source files, named 'karax', but file 'button.nim' is in a directory named 'examples' instead. This will be an error in the future.
      Hint: If 'examples' contains source files for building 'karax', rename it to 'karax'. Otherwise, prevent its installation by adding `skipDirs = @["examples"]` to the .nimble file.
  Verifying dependencies for karax@#head
 Installing karax@#head
   Building karax/karax/tools/karun using c backend
   Success: karax installed successfully.

nimble install karax
Downloading https://github.com/pragmagic/karax using git
   Warning: Package 'karax' has an incorrect structure. It should contain a single directory hierarchy for source files, named 'karax', but file 'button.nim' is in a directory named 'examples' instead. This will be an error in the future.
      Hint: If 'examples' contains source files for building 'karax', rename it to 'karax'. Otherwise, prevent its installation by adding `skipDirs = @["examples"]` to the .nimble file.
  Verifying dependencies for [email protected]
 Installing [email protected]
   Building karax/karax/tools/karun using c backend
   Success: karax installed successfully.

@EchoPouet
Copy link
Contributor Author

I can maybe improve the message.

@genotrance
Copy link
Contributor

I'll suggest that message should always be shown even if the user explicitly asks for #head. It should further explain the real problem with #head - if the user installs pkg1@#head at some point and then later on pkg2 requires [email protected] which is newer than when pkg1@#head was cloned, it won't ever get downloaded by Nimble. pkg2 will try to build with the old pkg1@#head and the build might fail leaving the user guessing what the problem is.

Nimble will assume that pkg1@#head in the repo is the latest so it totally throws off dependency version checking. Even if you force install [email protected], I think #head will be treated as newer.

Maybe we should always grumble that a package version #head is in use and warn the user that it could be out of date. Then of course, the user will want a way to update that to the latest #head and the headache never ends.

@EchoPouet
Copy link
Contributor Author

@genotrance I totally agree with you to add a warning messages when a user use #head. I can add it with the warning I have already added.

@dom96
Copy link
Collaborator

dom96 commented Jul 20, 2020

Nimble will assume that pkg1@#head in the repo is the latest so it totally throws off dependency version checking. Even if you force install [email protected], I think #head will be treated as newer.

That's not quite the case I don't think. The distinction here is with how Nim and Nimble handle dependencies. The Nim compiler does not read .nimble files to check versions but Nimble should and does AFAIK.

@EchoPouet
Copy link
Contributor Author

Nimble will assume that pkg1@#head in the repo is the latest so it totally throws off dependency version checking. Even if you force install [email protected], I think #head will be treated as newer.

That's not quite the case I don't think. The distinction here is with how Nim and Nimble handle dependencies. The Nim compiler does not read .nimble files to check versions but Nimble should and does AFAIK.

Sorry, but I don't understand your answer about the relationship between Nim and Nimble to manage dependencies (I don't very smart). Yes Nim doesn't care about .nimble files but we talk about Nimble when it uses to install, build or run a project. Personally, I rarely run Nim to build my project but rather nimble build. In this cases, using #head is a problem as @genotrance says.

Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

See remarks, but seems fine otherwise.

Comment on lines 221 to 223
else:
display("Warning:", "No corresponding tag found with required version.", Warning,
priority = HighPriority)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make the message here the same as above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You forgot this one :)

src/nimblepkg/download.nim Outdated Show resolved Hide resolved
@EchoPouet
Copy link
Contributor Author

Bump.

Can you merge it please ?

@dom96
Copy link
Collaborator

dom96 commented Jun 26, 2021

Can you make my suggested changes? :)

Co-authored-by: Dominik Picheta <[email protected]>
@EchoPouet
Copy link
Contributor Author

Sorry, it's good now.

@dom96 dom96 merged commit bb24fd0 into nim-lang:master Jun 27, 2021
@EchoPouet
Copy link
Contributor Author

Great. Thanks.

@EchoPouet EchoPouet deleted the warning-head-used branch October 27, 2021 10:55
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