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

Make should_show_banner work with julia 1.11 and nightly #1758

Merged
merged 3 commits into from
Aug 19, 2024

Conversation

lgoettgens
Copy link
Collaborator

I compared the backtraces of the __init__ call in the cases of using OtherPackage and using Package in 1.11.0-rc1, and the only difference is one single line number. So I added some code to the function that exploits this difference in the most robust way I can think of.

I think we should backport this change to AA 0.41.x afterwards, so that people using Oscar 1.1 with julia 1.11 in the near future (e.g. if julia 1.11 gets released before Oscar 1.2) don't get bloated with banners and instead use this updated code to suppress almost all of them (but one).

cc @benlorenz @lkastner

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.62%. Comparing base (3fc7d25) to head (ef41b00).
Report is 1 commits behind head on master.

Files Patch % Lines
src/utils.jl 81.25% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1758      +/-   ##
==========================================
- Coverage   87.62%   87.62%   -0.01%     
==========================================
  Files         117      117              
  Lines       30112    30121       +9     
==========================================
+ Hits        26386    26393       +7     
- Misses       3726     3728       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thofma
Copy link
Member

thofma commented Aug 14, 2024

@benlorenz are you happy with this?

Copy link
Collaborator

@benlorenz benlorenz left a comment

Choose a reason for hiding this comment

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

Sorry I somehow missed this while away. I don't really like that we are depending on internal line numbers and struct members but if it works I guess that is fine for now.

Maybe we could add a try/catch block around this to make sure this doesn't cause the packages to fail to load if some internals change in an incompatible way?

@thofma
Copy link
Member

thofma commented Aug 14, 2024

I think most of us would prefer a cleaner version, but I think no one has found one so far.

I like the idea of wrapping it around try/catch.

@lgoettgens
Copy link
Collaborator Author

I can do the try/catch thing next week once I am back in office

@lgoettgens
Copy link
Collaborator Author

I now added a try-catch construct around everything, that just returns true (aka is_loaded_directly = true aka banners will be printed) if something gets thrown. For ease of future debugging, the caught exception can be shown by executing ENV["JULIA_DEBUG"] = "AbstractAlgebra" before using SomePackage.

If there are no objections until tomorrow afternoon, I will proceed with merging this, and releasing a 0.41.11 on top of the 0.41.10 release with this backported to get this into Oscar 1.1.

Copy link
Collaborator

@benlorenz benlorenz left a comment

Choose a reason for hiding this comment

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

thanks!

@thofma thofma merged commit b6081d2 into Nemocas:master Aug 19, 2024
29 of 30 checks passed
@lgoettgens lgoettgens deleted the lg/banner-1.11 branch August 20, 2024 08:26
lgoettgens added a commit to lgoettgens/AbstractAlgebra.jl that referenced this pull request Aug 20, 2024
)

* Make `should_show_banner` work with julia 1.11 and nightly

* Enable banner tests on now working versions

* Add try-catch

(cherry picked from commit b6081d2)
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