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

Cache templates #6

Open
wants to merge 34 commits into
base: cache-task-not-lighthouse
Choose a base branch
from

Conversation

aminya
Copy link

@aminya aminya commented Jul 11, 2020

No description provided.

DeeDeeG and others added 30 commits May 30, 2020 10:23
We support Python 3 now. Use that in the default instructions,
but explain that `python2` or `python` will work in its place.
These overrides are very outdated.
(Haven't been updated since the day they were added, back in 2014.)
Even with these applied, Lintian still prints many warns/errors.

I think no-one has been running Lintian
against the .deb package for a while now.
Use this to find python for the verify-machine-requirements.js script.
.python-version specifies which version(s) of Python are in the PATH,
and which version runs when you run the python, python2, or python3
commands.

That said, it is overly specific for this repository's needs.
In order to specify any version, you must enter it down to the patch
level, e.g. 3.8.2; Major-minor versions aren't allowed, e.g. 2.7

If users want to add such a file on their own machines, they may...
But it's unneccesary for this repository to ship this file as if there
were specific "correct" versions of Python to use.

Any Python 2.7.x or 3.5.0+ will work at the moment.

There are other checks elsewhere in the project, such as in
script/bootstrap. These should be sufficient to inform users which
Python versions they can use. node-gyp will also tell you.
Log which Python commands were tried, and the results,
if no usable Python was found. Useful for debugging failures.
Python 2 is officially end-of-life. We can use Python 3 now.
in script/lib/verify-machine-requirements.js
Make sure a previously found version isn't erroneously logged,
by clearing the "fullVersion" variable before each new check.
…_release

no PR triggers on release builds
Miscellaneous python3-related updates and fixes
Faster atom.packages.getAvailablePackages
@aminya
Copy link
Author

aminya commented Jul 11, 2020

@DeeDeeG Please merge this and then rebase it or use git cherry-pick to only bring the last commit. Edit your previous PR to upstream to include the new change.

@aminya aminya force-pushed the cache_templates branch 2 times, most recently from 121c8d7 to b60ba40 Compare July 11, 2020 03:08
@DeeDeeG
Copy link
Owner

DeeDeeG commented Jul 11, 2020

Can we remove the OS parameter? We already get that info into the cache identifier from "$(Agent.OS)".

Edit: And maybe we should include the RestoreKeys stuff? https://docs.microsoft.com/en-us/azure/devops/pipelines/release/caching?view=azure-devops#restore-keys

@aminya
Copy link
Author

aminya commented Jul 11, 2020

No. Agent.OS gives different strings such as Window_NT depending on the agent that is being used. Here, we want the exact windows string.

@DeeDeeG
Copy link
Owner

DeeDeeG commented Jul 11, 2020

Pardon me for not understanding: Why do we want the nicer string? Is it for display?

Separate question: Why do we need it in the cache identifier, when "$(Agent.OS)" is unique for each OS?

I believe the effect is the same if no parameters are used. But it takes less code and might be more readable or simpler to maintain.

@aminya
Copy link
Author

aminya commented Jul 11, 2020

No problem. That is interpolated in the file names {{ parameters.OS }}. We want to point to the windows.yml file, not Windows_NT.yml.

  key: 'npm | "$(Agent.OS)" | "$(buildArch)" | package.json, package-lock.json, script/vsts/platforms/{{ parameters.OS }}.yml'

@DeeDeeG
Copy link
Owner

DeeDeeG commented Jul 11, 2020

Right, okay. Thanks.

I'll make sure that's in the PR. If you don't mind I'll just update the current one instead of closing/opening a new one?

@aminya
Copy link
Author

aminya commented Jul 11, 2020

Sorry. I fixed it.

@DeeDeeG
Copy link
Owner

DeeDeeG commented Jul 11, 2020

No worries, thanks. 👍

@DeeDeeG
Copy link
Owner

DeeDeeG commented Jul 11, 2020

This is part of the PR at upstream now: atom#21057

By the way, my PR to upstream is off of another branch: https://github.com/DeeDeeG/atom/tree/ci-use-official-cache-task

@aminya
Copy link
Author

aminya commented Jul 11, 2020

Thanks!

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.

5 participants