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

Move "breaking change" warning to build.jl #41

Closed
david-macmahon opened this issue May 15, 2024 · 9 comments · Fixed by #42
Closed

Move "breaking change" warning to build.jl #41

david-macmahon opened this issue May 15, 2024 · 9 comments · Fixed by #42

Comments

@david-macmahon
Copy link
Contributor

Every time HORIZONS is import-ed or using-ed it prints a warning about a breaking change. This can be very distracting, especially in a notebook. I would like to suggest moving this warning to deps/build.jl so that it is output only when the package is installed (or re-built via Pkg.build(")).

@PerezHz
Copy link
Owner

PerezHz commented May 23, 2024

Thanks for this suggestion; I agree we can make this a bit less verbose going forward. Feel free to submit a PR with the suggested change! 😄

@PerezHz
Copy link
Owner

PerezHz commented Jun 29, 2024

@david-macmahon this was fixed in principle by #42; feel free to reopen if it's not working as expected.

@david-macmahon
Copy link
Contributor Author

Unfortunately, it's only a partial victory. The warning does not appear when using HORIZONS, but it also does not appear on the console when doing ]add HORIZONS. It seems that Pkg.build is very successful at capturing stderr/stdout and logging it to build.log unless run in verbose mode. I tried a few different hacks, including @ccall write(2, ...) and open(/dev/sterr, "w"), but they all got captured and redirected to build.log.

I think it would be OK to revert this change (or decide to remove the warning altogether and/or move it to README.md). If we really want to show a warning on only the first using HORIZONS, we could use LocalPreferences.jl or Scratch.jl to track whether the warning has been displayed, but it hardly seems worth it.

This warning is most problematic (if one can really call it a "problem") when using HORIZONS .jl with Distributed.jl:

julia> using Distributed

julia> ws = addprocs(4);

julia> @everywhere using HORIZONS
┌ Warning: 
│ 
│     # Breaking change
│     Starting from v0.4.0 HORIZONS.jl connects to JPL via a HTTP API.
│     Previous versions used the telnet command line utility as an external dependency.
└ @ HORIZONS ~/.julia/dev/HORIZONS/src/HORIZONS.jl:35
      From worker 4:	┌ Warning: 
      From worker 4:	│ 
      From worker 4:	│     # Breaking change
      From worker 4:	│     Starting from v0.4.0 HORIZONS.jl connects to JPL via a HTTP API.
      From worker 4:	│     Previous versions used the telnet command line utility as an external dependency.
      From worker 4:	└ @ HORIZONS ~/.julia/dev/HORIZONS/src/HORIZONS.jl:35
      From worker 2:	┌ Warning: 
      From worker 2:	│ 
      From worker 2:	│     # Breaking change
      From worker 2:	│     Starting from v0.4.0 HORIZONS.jl connects to JPL via a HTTP API.
      From worker 2:	│     Previous versions used the telnet command line utility as an external dependency.
      From worker 2:	└ @ HORIZONS ~/.julia/dev/HORIZONS/src/HORIZONS.jl:35
      From worker 5:	┌ Warning: 
      From worker 5:	│ 
      From worker 5:	│     # Breaking change
      From worker 5:	│     Starting from v0.4.0 HORIZONS.jl connects to JPL via a HTTP API.
      From worker 5:	│     Previous versions used the telnet command line utility as an external dependency.
      From worker 5:	└ @ HORIZONS ~/.julia/dev/HORIZONS/src/HORIZONS.jl:35
      From worker 3:	┌ Warning: 
      From worker 3:	│ 
      From worker 3:	│     # Breaking change
      From worker 3:	│     Starting from v0.4.0 HORIZONS.jl connects to JPL via a HTTP API.
      From worker 3:	│     Previous versions used the telnet command line utility as an external dependency.
      From worker 3:	└ @ HORIZONS ~/.julia/dev/HORIZONS/src/HORIZONS.jl:35

@PerezHz PerezHz reopened this Jun 30, 2024
@PerezHz
Copy link
Owner

PerezHz commented Jun 30, 2024

Given that there has not been lots of complaints about moving on from telnet, I think it makes sense to move the warning to the README, so I'd propose to do just that.

@PerezHz
Copy link
Owner

PerezHz commented Jun 30, 2024

Can you confirm if the problem is solved in v0.4.8?

@david-macmahon
Copy link
Contributor Author

Thanks, that's great! Yes, the problem is solved/eliminated in v0.4.8. While checking README.md I noticed that it says the current stable release is v0.4.1, which is a bit out of date now. One other suggestion would be to switch the breaking change warning to a "GitHub markdown alert" (kind of like a Julia markdown admonition). This would display better on GitHub, but not quite as nicely on Julia (though not bad).

> [!WARNING]
> Be careful!

...looks like...

Warning

Be careful!

on GitHub and like │ [!WARNING] Be careful! in Julia. This warning will be going away eventually, so this is not a high priority IMHO, but I thought I'd mention it anyway. Thanks again!

@sefffal
Copy link

sefffal commented Jul 9, 2024

Appreciate this change!

@PerezHz
Copy link
Owner

PerezHz commented Jul 9, 2024

@david-macmahon thank you for the suggestions! I went for adding the current release badge from JuliaHub, which will updated by itself in the future, and also added the GitHub-style markdown warning. I'll close this issue now (do let us know, though, if there's anything else needed to be done here).

@PerezHz PerezHz closed this as completed Jul 9, 2024
@david-macmahon
Copy link
Contributor Author

Fantastic! Looks great to me, thanks!

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 a pull request may close this issue.

3 participants