Skip to content

extract julia directly to tool path to maintain mtimes#196

Merged
IanButterworth merged 18 commits intomasterfrom
ib/extract_directly
Jan 5, 2024
Merged

extract julia directly to tool path to maintain mtimes#196
IanButterworth merged 18 commits intomasterfrom
ib/extract_directly

Conversation

@IanButterworth
Copy link
Copy Markdown
Member

@IanButterworth IanButterworth commented Jan 4, 2024 •
edited
Loading

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
@IanButterworth
Revert "[debug] demo stat"
3714a58 
This reverts commit a34a04a.
@IanButterworth IanButterworth marked this pull request as ready for review January 4, 2024 06:30
@IanButterworth IanButterworth requested a review from a team as a code owner January 4, 2024 06:30
@IanButterworth IanButterworth mentioned this pull request Jan 4, 2024
Closed
@DilumAluthge
Copy link
Copy Markdown
Member

DilumAluthge commented Jan 4, 2024

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

DilumAluthge
DilumAluthge reviewed Jan 4, 2024
View reviewed changes
This was referenced Jan 4, 2024
Merged
Closed
@IanButterworth
Copy link
Copy Markdown
Member Author

IanButterworth commented Jan 4, 2024

Added mtime test which fails on master #200

@IanButterworth @DilumAluthge
add reference to PR
e6dda37 
Co-authored-by: Dilum Aluthge <[email protected]>
omus
omus reviewed Jan 4, 2024
View reviewed changes
omus
omus reviewed Jan 4, 2024
View reviewed changes
omus
omus approved these changes Jan 5, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@omus omus left a comment •
edited
Loading

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.

omus
omus reviewed Jan 5, 2024
View reviewed changes
omus
omus reviewed Jan 5, 2024
View reviewed changes
omus
omus approved these changes Jan 5, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

DilumAluthge 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.

@omus
Copy link
Copy Markdown
Contributor

omus commented Jan 5, 2024 •
edited
Loading

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
update to latest from #196
432da84
@IanButterworth
Copy link
Copy Markdown
Member Author

IanButterworth commented Jan 5, 2024

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

@DilumAluthge
Copy link
Copy Markdown
Member

DilumAluthge commented Jan 5, 2024

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
Copy Markdown
Member Author

IanButterworth commented Jan 5, 2024

Yeah, should be.

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

IanButterworth commented Jan 5, 2024 •
edited
Loading

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
Copy Markdown
Member Author

IanButterworth commented Jan 5, 2024

Released now https://github.com/julia-actions/setup-julia/releases/tag/v1.9.5

@omus
Copy link
Copy Markdown
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

Reviewers

@omus omus omus approved these changes

@DilumAluthge DilumAluthge DilumAluthge approved these changes

@SaschaMann SaschaMann Awaiting requested review from SaschaMann

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@IanButterworth @DilumAluthge @omus