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

extract julia directly to tool path to maintain mtimes #196

Merged
merged 18 commits into from Jan 5, 2024

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Jan 4, 2024

A bit of a hack to get julia installed with original mtimes to help JuliaLang/julia#50667

The comments explain the reasoning.

Also should be a bit faster as it avoids the cp action.

You can see here that 1.10.0 has the release mtimes

StatStruct("/Users/runner/hostedtoolcache/julia/1.10.0/x64/share/julia/stdlib/v1.10/MbedTLS_jll/src/MbedTLS_jll.jl" 
size: 2169 bytes device: 16777220 inode: 7005911 mode: 0o100644 (-rw-r--r--) nlink: 1 uid: 501 (runner) 
gid: 20 (staff) rdev: 0 blksz: 4096 blocks: 8 
mtime: 2023-12-25T20:50:13+0000 (9 days ago) ctime: 2024-01-04T06:25:27+0000 (8 seconds ago))

The added mtime test fails on master to illustrate the issue. #200

Closes #100
Closes #165

@IanButterworth IanButterworth force-pushed the ib/extract_directly branch 2 times, most recently from 672e081 to 1293a89 Compare January 4, 2024 05:37
@IanButterworth IanButterworth changed the title extract julia directly to tool path extract julia directly to tool path to maintain mtimes Jan 4, 2024
@IanButterworth IanButterworth force-pushed the ib/extract_directly branch 2 times, most recently from f869840 to f8eb0f4 Compare January 4, 2024 05:51
This reverts commit a34a04a.
@DilumAluthge
Copy link
Member

Can we add a test for this? E.g. do StatStruct in Julia (like in your example above) and check the mtime?

@IanButterworth
Copy link
Member Author

Added mtime test which fails on master #200

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
src/setup-julia.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@omus omus left a comment

Choose a reason for hiding this comment

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

Changes are reasonable to avoid having the tool-cache module from modifying the mtimes. My only concern is that the tc.cacheDir function is described as "Caches a directory and installs it to the tool cacheDir" so it is possible that a non-breaking change to tc.cacheDir such as making the tool path immutable would break this code. However, since the version of @actions/tool-cache is pinned until we update it we should be able to avoid such a scenario.

.github/workflows/common-tests.jl Outdated Show resolved Hide resolved
.github/workflows/common-tests.jl Outdated Show resolved Hide resolved
src/setup-julia.ts Outdated Show resolved Hide resolved
IanButterworth and others added 2 commits January 5, 2024 10:13
Co-authored-by: Curtis Vogt <curtis.vogt@gmail.com>
src/setup-julia.ts Outdated Show resolved Hide resolved
lib/setup-julia.js Outdated Show resolved Hide resolved
Copy link
Contributor

@omus omus left a comment

Choose a reason for hiding this comment

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

:chefkiss:

@DilumAluthge
Copy link
Member

Can we remove the tests from here, and keep the tests in #200, and merge #200 separately?

Reason: I am hoping that this PR will be a short-term solution; I'm hoping that the long-term solution will be for me to get a patch into upstream tc.cacheDir to add the ability to preserve mtimes. If we are able to get that feature accepted into upstream, then we can revert this PR. But if/when we revert this PR, I wouldn't want to revert the tests. If we keep the tests separate in #200, then later we can revert this PR without reverting #200.

@omus
Copy link
Contributor

omus commented Jan 5, 2024

I'm hoping that the long-term solution will be for me to get a patch into upstream tc.cacheDir to add the ability to preserve mtimes.

If we're changing tool-cache you may want to introduce a tc.cache function which provides a callback so we just do the install inside the callback. I foresee that this new function could be used by tc.cacheFile and tc.cacheDir.

IanButterworth added a commit that referenced this pull request Jan 5, 2024
@IanButterworth
Copy link
Member Author

Ok, #200 is updated. Probably best to merge that as failing first then rebase this

@DilumAluthge
Copy link
Member

This is great!

Is this the last change needed to make /compiled reusable in GitHub Actions CI when using Julia 1.10?

@IanButterworth
Copy link
Member Author

Yeah, should be.

@IanButterworth IanButterworth merged commit d3d61d9 into master Jan 5, 2024
43 checks passed
@IanButterworth IanButterworth deleted the ib/extract_directly branch January 5, 2024 17:49
@IanButterworth
Copy link
Member Author

IanButterworth commented Jan 5, 2024

I made a test release for 1.9.5 using bin/build-test-release.sh and tested it here IanButterworth/PowerMonitor.jl#2

with

uses: julia-actions/setup-julia@test/ib/1_9_5/releases/v1.9.5

And all platforms avoid re-precompilation once cached

@IanButterworth
Copy link
Member Author

@omus
Copy link
Contributor

omus commented Jan 5, 2024

Can we remove the tests from here, and keep the tests in #200, and merge #200 separately?

Reason: I am hoping that this PR will be a short-term solution; I'm hoping that the long-term solution will be for me to get a patch into upstream tc.cacheDir to add the ability to preserve mtimes. If we are able to get that feature accepted into upstream, then we can revert this PR. But if/when we revert this PR, I wouldn't want to revert the tests. If we keep the tests separate in #200, then later we can revert this PR without reverting #200.

Upstream tc.cacheDir PR to preserve mtimes: actions/toolkit#1617

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.

None yet

3 participants