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

Work around nightly breakage by re-allowing Julia.name := val from GAP #1006

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

lgoettgens
Copy link
Member

Resolves #1004.

This code was last touched in #803, where Julia.MODULE.name := val was allowed. In the recent julia nightly change JuliaLang/julia#54678, the implicit creation of global bindings was disabled, one can now only change the value of a global variable but not create a new one.

I can think of multiple ways on how to resolve this in GAP.jl:

  1. Use the workaround from Don't let setglobal! implicitly create bindings JuliaLang/julia#54678 to keep it working, i.e. add an explicit call to first create the global binding to GAP._setglobal. This is currently implemented in this PR.
  2. Keep everything as is and just adjust the tests. GAP.jl users adding new global variables to julia via Julia.MODULE.name := val will then run into the new error introduced in Don't let setglobal! implicitly create bindings JuliaLang/julia#54678. One could consider this as a breaking change to GAP.jl, but note that the julia devs didn't consider Don't let setglobal! implicitly create bindings JuliaLang/julia#54678 breaking.
  3. Only allow the implicit creation of global bindings via the method in 1. for the Julia.Main module, and either add our own error or direct to the julia error for all other modules. This was already discussed in Allow Julia.MODULE.x := 1 in GAP code, remove JuliaSetVal #803 (review).

Please let me know what you think about the possible options, and I will be happy to adapt this PR.

Copy link

codecov bot commented Jun 18, 2024

Codecov Report

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

Project coverage is 75.77%. Comparing base (2bdbe66) to head (ba47aa8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1006      +/-   ##
==========================================
- Coverage   75.81%   75.77%   -0.05%     
==========================================
  Files          51       51              
  Lines        4197     4202       +5     
==========================================
+ Hits         3182     3184       +2     
- Misses       1015     1018       +3     
Files Coverage Δ
src/utils.jl 50.00% <0.00%> (-1.62%) ⬇️

... and 1 file with indirect coverage changes

@ThomasBreuer
Copy link
Member

I have no strong preference.
Assignments in arbitrary modules are in principle possible, and it looks as if it is easier to allow them than to insert specific error messages. Thus the proposed solution 1. seems to be the simplest solution, and we can take it.

(Nevertheless, this is funny.
From the GAP side, assigning variables in arbitrary modules does not look special, but logically it is something different than usual variable assignments in GAP.
In the discussion of #803, we had more or less agreed that such assignments in modules different from Main are not a good idea, but since apparently the Julia people have no concerns, we can follow them.
Now the Julia people have changed their opinion, so the question arises whether we follow them again, or keep the old behaviour.)

@fingolfin fingolfin merged commit b0717f5 into oscar-system:master Jun 20, 2024
21 of 22 checks passed
@lgoettgens lgoettgens deleted the lg/setglobal branch June 20, 2024 13:55
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.

Failures in Julia nightly
3 participants