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

Introduce global git cache #1240

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ysangkok
Copy link
Contributor

@ysangkok ysangkok commented Jun 30, 2024

Description of the change

Another attempt at #1238. This introduces a global git cache which is used for
non-branches. That means that if you have a tag/commit specified on multiple
dependencies, it will only be cloned once.

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

Copy link
Member

@f-f f-f left a comment

Choose a reason for hiding this comment

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

Ah yes, this patch looks much better! The overall structure is good and I left a few minor comments about code organisation and naming

CI fails one test, but I'm hoping it's a glitch 😄

src/Spago/Config.purs Outdated Show resolved Hide resolved
src/Spago/Git.purs Outdated Show resolved Hide resolved
src/Spago/Git.purs Outdated Show resolved Hide resolved
src/Spago/Git.purs Outdated Show resolved Hide resolved
src/Spago/Config.purs Outdated Show resolved Hide resolved
src/Spago/Command/Fetch.purs Outdated Show resolved Hide resolved
src/Spago/Command/Fetch.purs Outdated Show resolved Hide resolved
src/Spago/Command/Fetch.purs Outdated Show resolved Hide resolved
src/Spago/Command/Fetch.purs Outdated Show resolved Hide resolved
src/Spago/Command/Fetch.purs Outdated Show resolved Hide resolved
@f-f
Copy link
Member

f-f commented Jul 1, 2024

Ah right, CI fails because this change is correct 🙂
In the can't install (uncached) dependencies if offline test we were verifying that git repos never get cached, but with this change we hit the cache when trying to clone the same repo twice. We'll have to clone a different version of either in there, and/or add a test where we clone a branch.

@ysangkok
Copy link
Contributor Author

ysangkok commented Jul 1, 2024

@f-f I applied your suggested changes, and also an mkdirp for the deeper directory hierarchy. The reason I joined package.git with package.ref is because there is no synchronization outside the filesystem, so if there is a subdirectory, I am worried that the more work happen between the call to exists and the mkdirp and the moveSync, the more likely threads are to interleave any of this, which wouldn't work. Ideally, the move should be called unconditionally, and it fails because of an existing destination, the failure could be ignored. That would be correct on many Linux filesystems since moves are atomic. Now that we have three file system operations, it is not atomic. Not sure what to do about this.

Copy link
Member

@f-f f-f left a comment

Choose a reason for hiding this comment

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

Now that we have three file system operations, it is not atomic. Not sure what to do about this.

We can catch filesystem errors, see the suggestions

Note: we'll still need some changes to the tests, see my previous comment

src/Spago/Command/Fetch.purs Outdated Show resolved Hide resolved
src/Spago/Command/Fetch.purs Outdated Show resolved Hide resolved
@ysangkok
Copy link
Contributor Author

ysangkok commented Jul 2, 2024

I don't know how to change the version of either, so I tried to put identity instead, but it doesn't seem to have worked. I wonder if identity is somehow already in the cache. Not sure what other package to use.

test/Spago/Install.purs Outdated Show resolved Hide resolved
@ysangkok
Copy link
Contributor Author

ysangkok commented Jul 3, 2024

@f-f Let me know if there are any remaining tasks for this, please! I have fixed the formatting.

@f-f
Copy link
Member

f-f commented Jul 4, 2024

CI fails because on Windows (and only there) the folder in the cache is not found when we go copy it:

Error: ENOENT: no such file or directory, lstat 'C:\\Users\\runneradmin\\AppData\\Local\\spago-nodejs\\Cache\\git\\https%3a%2f%2fgithub.com%2fpurescript%2fpurescript-either.git\\af655a04ed2fd694b6688af39ee20d7907ad0763'
    at Object.lstatSync (node:fs:1666:3)
    at Object.lstatSync (D:\\a\\spago\\spago\\node_modules\\graceful-fs\\polyfills.js:318:34)
    at statFunc (D:\\a\\spago\\spago\\node_modules\\fs-extra\\lib\\util\\stat.js:24:20)
    at getStatsSync (D:\\a\\spago\\spago\\node_modules\\fs-extra\\lib\\util\\stat.js:25:19)
    at Object.checkPathsSync (D:\\a\\spago\\spago\\node_modules\\fs-extra\\lib\\util\\stat.js:64:33)
    at Object.copySync (D:\\a\\spago\\spago\\node_modules\\fs-extra\\lib\\copy\\copy-sync.js:27:38)
    at file:///D:/a/spago/spago/output/Spago.FS/foreign.js:9:54
    at runSync (file:///D:/a/spago/spago/output/Effect.Aff/foreign.js:88:20)
    at run (file:///D:/a/spago/spago/output/Effect.Aff/foreign.js:326:22)
    at file:///D:/a/spago/spago/output/Effect.Aff/foreign.js:346:19 {
  errno: -4058,
  syscall: 'lstat',
  code: 'ENOENT',
  path: 'C:\\\\Users\\\\runneradmin\\\\AppData\\\\Local\\\\spago-nodejs\\\\Cache\\\\git\\\\https%3a%2f%2fgithub.com%2fpurescript%2fpurescript-either.git\\\\af655a04ed2fd694b6688af39ee20d7907ad0763'
}

I had weird encounters with windows not writing files to disk in a timely manner, but it would be strange if it would do it for entire folders.
I wonder if we're writing to a place and trying to read from another, or if some of the characters we use in the escaping are not good

@ysangkok
Copy link
Contributor Author

ysangkok commented Jul 8, 2024

I can't seem to reproduce this failure locally. So not sure how to proceed. I could insert some delays between tests, but that wouldn't be an acceptable permanent solution.

image

@f-f
Copy link
Member

f-f commented Jul 8, 2024

Ah that's strange - I wonder if this is about Node versions? In CI we're using a pretty old one, 18:

node-version: 18

Bumping it up to 22 might just fix it.
If not I'll try to take a look on my Windows machine sometime in the next days

@ysangkok
Copy link
Contributor Author

Is there a way to get the test runner to just run a single test? That would help with diagnosing whether it's the state carried over between tests that is messing up the failing test.

@f-f
Copy link
Member

f-f commented Jul 23, 2024

Yes! You can swap the Spec.it of that test for Spec.itOnly

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

2 participants