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

Fixes #798 (relying on undefined behaviour) #802

Merged
merged 3 commits into from May 19, 2020
Merged

Fixes #798 (relying on undefined behaviour) #802

merged 3 commits into from May 19, 2020

Conversation

ghost
Copy link

@ghost ghost commented May 19, 2020

This PR fixes #798.
I'm not sure if this is the best way, but it requires the least amount of changes to the code. It's required for nim-lang/Nim#14390 .

I'm not 100% sure that I fixed all places where that behaviour was used, so please comment if you find any issues.

@@ -464,6 +464,7 @@ proc getInstalledPkgs*(libsDir: string, options: Options):
pkg = readPackageInfo(nimbleFile, options, onlyMinimalInfo=false)
except ValidationError:
let exc = (ref ValidationError)(getCurrentException())
pkg = exc.pkgInfo
Copy link
Author

Choose a reason for hiding this comment

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

By the way - this procedure (getInstalledPkgs) is not used anywhere, is that okay?

hint: string = "", warnAll = false) =
raise newValidationError(msg, warnInstalled, hint, warnAll)
hint = "", warnAll = false) =
raise PackageInfo().newValidationError(msg, warnInstalled, hint, warnAll)
Copy link
Author

Choose a reason for hiding this comment

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

This might not look correct - but in fact it is - it's only used this way in validatePackageName which is only called from readPackageInfo where it's not covered in a try/except block so it won't make sense (and will require changing more code) to pass PackageInfo() there.
I also could've made a new newValidationError proc without info: PackageInfo, but I wanted to keep my changes as minimal as possible

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! I've got an idea that may make this change easier, are you happy to implement it?

basically just change readPackageInfo to take a var PackageInfo instead of returning it.

But really, it's quite strange that we are using pkg once it's failed validation, the real fix is probably to stop doing that.

@dom96 dom96 merged commit 4541c13 into nim-lang:master May 19, 2020
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.

Nimble doesn't work with --gc:arc
1 participant