Skip to content

Pkg.Registry.update(): add throw_on_error kwarg #4245

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DilumAluthge
Copy link
Member

@DilumAluthge DilumAluthge commented May 22, 2025

Fixes #4244

This PR adds the optional throw_on_error kwarg to the Pkg.Registry.update() function.

The default value is throw_on_error = false, which preserves the existing behavior by default.

However, if the user opts-in by specifying throw_on_error = true, then if Pkg.Registry.update() encounters an error, we will throw an exception.

The name throw_on_error can be bikeshedded.

@DilumAluthge
Copy link
Member Author

Hmmm. I think I should wait until the end, instead of throwing immediately.

@KristofferC
Copy link
Member

I don't really like this because Registry.update is automatically called in various places and you don't want the pkg operation to completely fail if one of the registries fail to update. For example, if you do Pkg.add("Plots") and some test registry fails to update you still want to install plots.

@Keno
Copy link
Member

Keno commented May 23, 2025

In particular, the network might be down, but you still want to do the Project.toml modification.

@DilumAluthge
Copy link
Member Author

How about we set throw_on_error = false by default? So then, by default, people will get the existing behavior, but people can opt-in by specifying throw_on_error = true?

@omus I think that would still solve your use case, right?

@DilumAluthge DilumAluthge changed the title WIP: Pkg.Registry.update(): throw if an error is encountered WIP: Pkg.Registry.update(): add throw_on_error kwarg May 23, 2025
@DilumAluthge
Copy link
Member Author

How about we set throw_on_error = false by default? So then, by default, people will get the existing behavior, but people can opt-in by specifying throw_on_error = true?

I have implemented this.

@DilumAluthge DilumAluthge changed the title WIP: Pkg.Registry.update(): add throw_on_error kwarg Pkg.Registry.update(): add throw_on_error kwarg May 23, 2025
@DilumAluthge DilumAluthge marked this pull request as ready for review May 23, 2025 17:23
@DilumAluthge DilumAluthge requested review from KristofferC and omus May 23, 2025 17:23
Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

I'm onboard with this change. My in my use case I want this to fail fast to avoid spending time on adding packages when it will eventually fail

Comment on lines 556 to +559
@error warn_str
if throw_on_error
Pkg.Types.pkgerror(warn_str)
end
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to both have an @error and an exception?

@@ -392,7 +392,7 @@ function update(; name=nothing, uuid=nothing, url=nothing, path=nothing, linked=
update([RegistrySpec(; name, uuid, url, path, linked)]; kwargs...)
end
end
function update(regs::Vector{RegistrySpec}; io::IO=stderr_f(), force::Bool=true, depots = [depots1()], update_cooldown = Second(1))
function update(regs::Vector{RegistrySpec}; io::IO=stderr_f(), force::Bool=true, depots = [depots1()], update_cooldown = Second(1), throw_on_error = false)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe could be:

Suggested change
function update(regs::Vector{RegistrySpec}; io::IO=stderr_f(), force::Bool=true, depots = [depots1()], update_cooldown = Second(1), throw_on_error = false)
function update(regs::Vector{RegistrySpec}; io::IO=stderr_f(), force::Bool=true, depots = [depots1()], update_cooldown = Second(1), strict::Bool=false)

We already have a Pkg.precompile(; strict::Bool=false) which reminds me of this

@DilumAluthge
Copy link
Member Author

Maybe could be:

We already have a Pkg.precompile(; strict::Bool=false) which reminds me of this

I think that strict would be fine, especially if we already use strict in other places.

So the options that we are considering are:

  1. throw_on_error
  2. strict

Are there any other options that we can think of?

@github-project-automation github-project-automation bot moved this to New in Pkg.jl Jul 2, 2025
@IanButterworth IanButterworth moved this from New to In review in Pkg.jl Jul 2, 2025
@IanButterworth
Copy link
Member

I think just throw is enough. I think that's used elsewhere.

@IanButterworth IanButterworth moved this from In review to In progress in Pkg.jl Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Registry update failures should throw or return status
5 participants