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

Do not use system.NimVersion for nim.nimble #21313

Closed
wants to merge 1 commit into from

Conversation

yyoncho
Copy link
Contributor

@yyoncho yyoncho commented Jan 30, 2023

  • That version is evaluaed in the context of nim version used by nimble

- That version is evaluaed in the context of `nim` version used by `nimble`
@metagn
Copy link
Collaborator

metagn commented Jan 31, 2023

Maybe from lib/system import NimVersion?

@Araq
Copy link
Member

Araq commented Jan 31, 2023

Nimble delegates Nimscript evaluation to Nim so this should be fine.

@yyoncho
Copy link
Contributor Author

yyoncho commented Jan 31, 2023

Nimble delegates Nimscript evaluation to Nim so this should be fine.

@Araq indeed. If you do the following

choosenim "#version-1-2"
nimble install

you will end up with the following package in the pkgs2 directory: nim-1.2.19-f49de7c8bd455fb50778683448ac4d9f5fc3e029

and the following output:

...
  target type: 'uint' (8 bytes)
  source type: 'set[ValidationErrorKind]' (1 byte) [CastSizes]
  Verifying dependencies for [email protected]
 Installing [email protected]
  Success:  nim installed successfully.

That will happen no matter what is the Nim version you have checked out.

@Araq
Copy link
Member

Araq commented Feb 1, 2023

I don't understand, why is this a problem?

@yyoncho
Copy link
Contributor Author

yyoncho commented Feb 1, 2023

I don't understand, why is this a problem?

@Araq, it is a problem when you have something like requires "nim == 1.7.1". Then when nimble tries to install the packages it will fail because the validation of the package will fail because nimble will think that the package it is trying to install is not 1.7.1. That will happen if local nim is not 1.7.1

See nim-lang/nimble#1017 .

@Araq
Copy link
Member

Araq commented Feb 1, 2023

We will forget to update nim.nimble though so this solution is pretty bad. Any alternatives possible? How about nim.nimble doesn't specify the version and Nimble uses git tags in order to determine a version? That's what Nimble should have done to begin with, anyway.

@yyoncho
Copy link
Contributor Author

yyoncho commented Feb 1, 2023

How about nim.nimble doesn't specify the version and Nimble uses git tags in order to determine a version?

I will check if we can work with that. It is a bit unclear to me what version should I use in case the revision at hand is not tagged.

In general, I can patch nimble to handle nim package as a special case(e. g. go and read that file) but we have to figure out what would work.

@Araq
Copy link
Member

Araq commented Feb 1, 2023

You don't have to special case Nim for this, Nimble should accept nimble files lacking version and then use hg/git tags instead.

@yyoncho yyoncho closed this Feb 21, 2023
yyoncho added a commit to yyoncho/Nim that referenced this pull request Feb 21, 2023
yyoncho added a commit to yyoncho/Nim that referenced this pull request Feb 21, 2023
Araq pushed a commit that referenced this pull request Mar 2, 2023
narimiran pushed a commit that referenced this pull request Mar 7, 2023
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
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.

3 participants