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

Prevent compiler warning without panicking #1084

Merged
merged 3 commits into from
Oct 27, 2024

Conversation

christiangnrd
Copy link
Contributor

@christiangnrd christiangnrd commented Oct 25, 2024

My attempt at resolving #1068.

The source of the panic seems to be an assertion that was added purely to remove a compiler warning, so I turned it into a warning because I don't think an empty version field should prevent updating to a latest PR.

TODO: If people decide this is an acceptable solution, I'll improve the warning message with potential causes.

One cause is when an installed channel has an empty version field due to not being able to get the version number from a non-codesigned executable on macOS #1068 (comment). It could also be that a particular PR was merged #1068 (comment).

@IanButterworth
Copy link
Member

not being able to get the version number from a non-codesigned executable on macOS

Good call. Although I tried adding a pr channel, julia didn't load because of this.
I feel we should make that clearer to the user somehow.

@davidanthoff
Copy link
Collaborator

So if the underlying issue at the moment is that pr channels just don't work, then maybe we should (in the very short run) disable that feature on MacOS, and then try to properly fix #1000.

But I think we should not just start to ignore these empty versions at this place here, instead we should probably just abort installation of a PR channel if we can't launch the executable to extract the version in the first place.

src/command_update.rs Outdated Show resolved Hide resolved
@christiangnrd
Copy link
Contributor Author

So if the underlying issue at the moment is that pr channels just don't work, then maybe we should (in the very short run) disable that feature on MacOS, and then try to properly fix #1000.

But I think we should not just start to ignore these empty versions at this place here, instead we should probably just abort installation of a PR channel if we can't launch the executable to extract the version in the first place.

While I agree #1086 is a much better solution. I think that this PR would be a better very-short-term solution than disabling the feature for mac users because it prevents a crash when trying to update, and still allows to download/update a PR branch. Users can then manually codesign the downloaded PR if they want.

Co-authored-by: Christian Guinard <[email protected]>
Copy link
Collaborator

@davidanthoff davidanthoff left a comment

Choose a reason for hiding this comment

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

You convinced me :)

@davidanthoff davidanthoff merged commit 8825a23 into JuliaLang:main Oct 27, 2024
25 checks passed
@christiangnrd christiangnrd deleted the emptyversion branch October 27, 2024 01:16
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