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

Cach2 instead of lighthouse #11

Closed
wants to merge 1 commit into from
Closed

Cach2 instead of lighthouse #11

wants to merge 1 commit into from

Conversation

aminya
Copy link
Member

@aminya aminya commented Jul 6, 2020

No description provided.

@DeeDeeG
Copy link
Member

DeeDeeG commented Jul 6, 2020

A bit tangential to this: I figured out the Lighthouse caching thing. See #13.

full comment (click to expand)

Not to be annoying, but I think I just figured out the Lighthouse cache thing.

Should I post a PR and instructions for how to set it up here?

Edit: Check this commit with the fix.

We get the proper ID from the "fullyQualifiedID" here: feeds.dev.azure.com/${your_organization_name_here}/_apis/packaging/feeds?api-version=5.0-preview, so: https://feeds.dev.azure.com/atomcommunity/_apis/packaging/feeds?api-version=5.0-preview (you need admin authorization to the Azure DevOps organization to view that).

The description should say "Default feed for ${organization}" i.e. "Default feed for atomcommunity". If the description says something else or "null", then it's the wrong feed.

Edit 2: Working run with Lighthouse set up properly. It's a cache miss since this is the first time it's being run, but there's no warnings. https://dev.azure.com/DeeDeeG/b/_build/results?buildId=65&view=logs&j=e2cf4b02-5697-54ad-cf7c-fc2a840d53af&t=cf8e3cfb-de27-5959-c474-2e21b7dd5cf4

@aminya
Copy link
Member Author

aminya commented Jul 6, 2020

@DeeDeeG Yes, Sure! Make a pull request to this repository so we can see how it works

@DeeDeeG
Copy link
Member

DeeDeeG commented Jul 9, 2020

On topic again: I think if we cache the entire repository, it will overwrite the entire repository the next time around when the "cache" is restored. I suppose we could do a git reset --hard to make sure everything tracked by git is current... But if any package.json or package-lock.json in the whole repo changed... or any substantial piece of code... I feel like Atom truly does need to be rebuilt then, for CI purposes.

(Caching/restoring the entire project is also rather slow!)

Boostrapping only sets up the main node_modules, script/node_modules and apm/node_modules folders.

https://github.com/atom/atom/blob/110b05baf7cb1e50ad7d679173e6fc8f698cab0d/script/bootstrap#L37-L44

  • This does npm install from in script/ (script/bootstrap runs from its own dir, which is script/)
  • Does npm install from apm/ (uses the cwd option to "change working directory" into apm/)
  • Then uses apm to install the packages in the project root ([repo_root]/node_modules, including the bundled packages in [repo_root]/packages, such as atom-dark-ui.)

So I think we should only cache those three node_modules folders for the bootstrap.

Everything else is Atom code that might have changed, or stuff from the build process, including the out/ folder with the atom installer, etc... Which is a separate thing from bootstrapping.

It's true that the Cache@v2 step only allows one file or folder to be cached, but we can run the cache three times, one per node_modules folder.

[I had this comment sitting open as a draft for a while. I posted my attempt at using Cache@v2 here: #30.]

@aminya
Copy link
Member Author

aminya commented Jul 9, 2020

I will close this in favor of #30

@aminya aminya closed this Jul 9, 2020
@DeeDeeG
Copy link
Member

DeeDeeG commented Jul 9, 2020

By the way, caching on Linux will be much faster if we can get past Electron 5. The electron-mksnapshot binary for electron v5 on Linux is massive (433 MB) and gets downloaded at least once or twice during npm install in the script/ directory and in the [repo_root] directory.

@aminya aminya added the CI label Jul 9, 2020
@aminya aminya linked an issue Jul 9, 2020 that may be closed by this pull request
@aminya aminya removed a link to an issue Jul 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants