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

Fix sandbox call on 1.11-beta #87

Merged
merged 8 commits into from
Jun 11, 2024
Merged

Conversation

kbarros
Copy link
Contributor

@kbarros kbarros commented May 16, 2024

As a possible fix to #86, one could modify the arguments to sandbox at the call site, depending on a static version comparison. This would minimize code duplication by reusing the rest of the code inside src/julia-1.9. In this branch, tests pass for me locally on both 1.10 and 1.11-beta.

Copy link

codecov bot commented May 16, 2024

Codecov Report

Attention: Patch coverage is 79.34783% with 19 lines in your changes missing coverage. Please review.

Project coverage is 39.06%. Comparing base (621a776) to head (b8f9be5).

Files Patch % Lines
src/julia-1.11/activate_set.jl 70.73% 12 Missing ⚠️
src/julia-1.11/common.jl 84.09% 7 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #87       +/-   ##
===========================================
- Coverage   73.91%   39.06%   -34.85%     
===========================================
  Files          25       28        +3     
  Lines         621      709       +88     
===========================================
- Hits          459      277      -182     
- Misses        162      432      +270     

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

@kbarros
Copy link
Contributor Author

kbarros commented Jun 2, 2024

Is this blocked on the Codecov failure? This PR just removes one of the arguments to sandbox when Julia 1.11 is detected. Any suggestions for pushing the PR forward?

@oxinabox
Copy link
Member

oxinabox commented Jun 3, 2024

Sorry no this is blocked by me not seeing it til now.
Don't hesitate to bump PRs after a week or so, its very easy for people maintaining many packages to not see a notification/email.

Having given this some thought,
I think the better thing to do is to copy julia-1.9 into a julia-1.11 folder, and make the change there.
We used to have a lot of these branches in the code before and it got pretty confusing once you get enough of them.
Which is how we ended up with the current multiple folders solution (with an multiple branches solution between the two)
As I wouldn't be surprised to need more for 1.11, with the new workspaces functionality.
(But we won't see that til some one tried TestEnv in somehing using it extensively and it breaks).

@kbarros
Copy link
Contributor Author

kbarros commented Jun 4, 2024

I have updated the PR as you suggested. The new commits undo all modifications to the julia-1.9/ folder, and create a new directory julia-1.11/ with the minimal fixes. Tests pass locally on 1.10 and 1.11.0-beta2. Merging this PR will hopefully unblock julia-vscode/TestItemRunner.jl#72.

@davidanthoff
Copy link
Member

@oxinabox ready to merge now?

@davidanthoff
Copy link
Member

@oxinabox could you maybe make me a co-maintainer of this repo so that I can merge PRs like this one?

@kbarros
Copy link
Contributor Author

kbarros commented Jun 8, 2024

Gentle ping

@oxinabox
Copy link
Member

Sorry I was away for a bit.
Will add David as a comaintainer. I thought I did so a year ago.

This needs to also add 1.10 and 1.11 to the CI matrix

@kbarros
Copy link
Contributor Author

kbarros commented Jun 10, 2024

I tried naively adding 1.11 to the CI matrix. This throws an error, since 1.11 hasn't been released. How to proceed?


Update: From this documentation, the correct syntax seems to be ^1.11.0-beta2. Now the only remaining CI failure is on Julia 1.4, but that seems unrelated to this PR.

I think this PR is ready to merge.

@davidanthoff
Copy link
Member

@oxinabox I think I still don't have write access so that I can merge things?

@oxinabox
Copy link
Member

I am good for this to be merged, I am going to leave it to David to actually merge because I want to check I setup his permissions right, since i failed the first time

@davidanthoff davidanthoff merged commit d65333e into JuliaTesting:main Jun 11, 2024
61 of 62 checks passed
@kbarros kbarros deleted the beta_fix branch June 11, 2024 03:57
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