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

Use Scratch.jl for GAP root #825

Merged
merged 1 commit into from
Oct 1, 2022
Merged

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Sep 29, 2022

This also forces a full regeneration of our custom GAP root dir each time the package loads, instead of only during precompilation. This adds ~130 milliseconds startup overhead for me, of which 90% are spent in finding working C/C++ compilers... Not great, but with Scratch.jl any of the directories could move, so we kinda have to do it this way, as far as I can tell (Polymake.jl also does it this way). Perhaps I could at least cache the C/C++ compiler values somehow, but for now I just want to have this working.

Resolves #822
Resolves #728

@fingolfin
Copy link
Member Author

This introduces a minor regression which I have not yet understood: upon using GAP, if GAP was already precompiled, all is fine; but if GAP gets precompiled then we don't show the GAP banner anymore. Huh?

@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Merging #825 (cfd6e24) into master (df089c9) will increase coverage by 3.49%.
The diff coverage is 76.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #825      +/-   ##
==========================================
+ Coverage   78.05%   81.54%   +3.49%     
==========================================
  Files          48       48              
  Lines        3686     3680       -6     
==========================================
+ Hits         2877     3001     +124     
+ Misses        809      679     -130     
Impacted Files Coverage Δ
src/GAP.jl 82.35% <75.00%> (+0.17%) ⬆️
src/setup.jl 81.33% <75.00%> (+81.33%) ⬆️
src/packages.jl 64.15% <100.00%> (+0.68%) ⬆️

@ThomasBreuer
Copy link
Member

@fingolfin I cannot reproduce the problem with the GAP banner.
I have checked out this branch and called Pkg.resolve().
After starting Julia and calling using GAP, the GAP banner is shown in my Julia session, no matter whether the message Info: Precompiling GAP is shown or not.

Copy link
Member

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks.

@fingolfin
Copy link
Member Author

@ThomasBreuer which Julia version are you using? For me the problem of the missing banner only occurs in Julia 1.8 -- and indeed, the hack we use (I think perhaps invented by @thofma ?) checks for a function _tryrequire_from_serialized -- and starting with Julia 1.8 this function is called in more places. So I am guessing this code may need to be adapted to work for Julia 1.8.

@thofma
Copy link
Contributor

thofma commented Oct 1, 2022

Yes, this was my doing. I can have a look.

@thofma
Copy link
Contributor

thofma commented Oct 1, 2022

I would suggest you merge this here and I will fix it in a followup PR.

@fingolfin fingolfin merged commit 91f1169 into oscar-system:master Oct 1, 2022
@fingolfin fingolfin deleted the mh/Scratch branch October 1, 2022 17:03
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.

Move GAPROOT out of package directory use Scratch.jl and/or RelocatableFolders.jl for gaproot, other stuff
3 participants