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

refactor: disable julia startup file for julia package update #983

Merged

Conversation

Laura7089
Copy link
Contributor

@Laura7089 Laura7089 commented Nov 15, 2024

What does this PR do

This PR adds the --startup-file=no argument to the julia invocation for updating Julia packages. See the upstream doc.

This is useful because if Julia has been recently updated (eg. with the juliaup step), then old versions of packages configured with using directives in ~/.julia/config/startup.jl may fail to precompile or load with the new version, in turn causing the package update step to fail unnecessarily. By skipping the startup file load, broken or outdated packages will not attempt to be loaded.

Standards checklist

  • The PR title is descriptive
  • I have read CONTRIBUTING.md
  • Optional: I have tested the code myself
  • [ ] If this PR introduces new user-facing messages they are translated

@SteveLauC
Copy link
Member

Hi, thanks for the PR!

I would like to know if it would affect the update in other cases by not loading this startup.jl file?

@SteveLauC SteveLauC closed this Nov 16, 2024
@SteveLauC SteveLauC reopened this Nov 16, 2024
@SteveLauC
Copy link
Member

Sorry for accidentally closing the PR. 😵‍💫

@Laura7089
Copy link
Contributor Author

It's difficult to answer that - there are almost certainly cases where people have unusual configuration in that file without which their Julia interpreter wouldn't work, but I'd confidently guess that the using-breakage case is much more common since that's a basic function of the feature. If any breakage is a concern, perhaps it could be gated behind a configuration flag?

@SteveLauC
Copy link
Member

Thanks for the explanation!

If any breakage is a concern, perhaps it could be gated behind a configuration flag?

Yeah, adding a configuration entry for this makes sense:

[julia]
# If true, Topgrade will pass the `--startup-file=yes` option to Julia when updating it.
#
# <!!!Here, please add the use case where this option would be helpful!!!>
# startup_file = false

We need a new [julia] section, and a startup_file (you can use other names if you prefer) config entry in the example config file. This new section will be added to this struct ConfigFile type, you can add such a helper function to struct Config to make accessing this config easier.

@Laura7089 Laura7089 force-pushed the refactor/julia-package-startupfile branch from 38fd63a to 75cce22 Compare November 16, 2024 13:58
@Laura7089
Copy link
Contributor Author

Laura7089 commented Nov 16, 2024

I've set the flag to default to true to mimic retain the (current at time of writing) default behaviour of julia. Is this appropriate or should we prefer the "fixed" case?

@SteveLauC
Copy link
Member

or should we prefer the "fixed" case?

Emm, what does the fixed case look like? You mean the change presented in this commit? I am not a user of Julia, but based on our discussion, making it configurable and don't change the current behavior is generally preferred, just in case.

Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Just one nit:)

src/config.rs Outdated Show resolved Hide resolved
@Laura7089 Laura7089 requested a review from SteveLauC November 18, 2024 12:11
Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Thanks!

@SteveLauC SteveLauC merged commit 202897b into topgrade-rs:main Nov 19, 2024
12 checks passed
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.

2 participants