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

[GitHub Actions] run manifest_build.py directly on Ubuntu 18.04 #19580

Merged
merged 1 commit into from Oct 14, 2019

Conversation

foolip
Copy link
Member

@foolip foolip commented Oct 8, 2019

Fixes #19380.

Running faster increases the risk that the GitHub search API can't
find the pull request yet, so add retry.py with a --delay value that
with 5 iterations adds up to a total of 15 minutes waiting.

@foolip
Copy link
Member Author

foolip commented Oct 8, 2019

In 192914f#r35409518 @jgraham asked:

Wouldn't an alternative be just to upload this image to dockerhub, and they we could also avoid needing to instal zstd or other deps?

That would indeed be possible, and might be 15-30 seconds faster than this if the docker image is cached, but the dependencies are so few/simple here that I'm not sure the extra work of maintaining/updating the image are worth it.

@jugglinmike thoughts?

Fixes #19380.

Running faster increases the risk that the GitHub search API can't
find the pull request yet, so add retry.py with a --delay value that
with 5 iterations adds up to a total of 15 minutes waiting.
@foolip foolip force-pushed the foolip/manifest-sans-docker branch from 0fb3ee9 to 344fc3a Compare October 9, 2019 15:39
@foolip
Copy link
Member Author

foolip commented Oct 9, 2019

Comparing these two checks gives an idea about the speed improvement:
https://github.com/web-platform-tests/wpt/runs/253789228 (master)
https://github.com/web-platform-tests/wpt/pull/19580/checks?check_run_id=253821136

The original setup took 218s in "Run manifest_build.py", while this PR took 94s for installing dependencies and running the same step. It didn't actually upload the assets and create the release, but that part was only 2s in the original setup. So, one can shave off 2 minutes or so.

At the end of the day the speedup is not hugely important, a more direct benefit is that the logs are cleaner as they don't start with a whole lot of docker fetching.

@gsnedders
Copy link
Member

I am in favour, but I leave the review to @jgraham.

@jgraham jgraham merged commit 2277f7c into master Oct 14, 2019
@foolip foolip deleted the foolip/manifest-sans-docker branch October 14, 2019 11:33
@foolip
Copy link
Member Author

foolip commented Oct 14, 2019

Alright, https://github.com/web-platform-tests/wpt/runs/259065754 worked and was not all too slow. Now let's see how long it takes before the job fails when fetching from pythonhosted.org and what to do then...

foolip added a commit that referenced this pull request Oct 14, 2019
This is like #19580,
but the dependencies of this job are even more modest and are already
fulfilled by the base ubuntu-18.04 image.
foolip added a commit that referenced this pull request Oct 21, 2019
In #19580 retry for
manifest generation was introduced, to make it more robust in case of
network errors of the GitHub search API not indexing the merged PR
fast enough.

However, an accidental commit to master showed that this won't work:
https://github.com/web-platform-tests/wpt/runs/265191690

It first failed with "No PR found for 9e1d0a2"
because there was no PR, just a direct push to master. Retries then
failed with "File exists: '/home/runner/meta'".

Use a new temporary directory every time to deal with this. Don't
attempt to remove the directory when done as this script is run in a
CI system where all state is thrown away on completion.
foolip added a commit that referenced this pull request Oct 21, 2019
In #19580 retry for
manifest generation was introduced, to make it more robust in case of
network errors or the GitHub search API not indexing the merged PR
fast enough.

However, an accidental commit to master showed that this won't work:
https://github.com/web-platform-tests/wpt/runs/265191690

It first failed with "No PR found for 9e1d0a2"
because there was no PR, just a direct push to master. Retries then
failed with "File exists: '/home/runner/meta'".

Use a new temporary directory every time to deal with this. Don't
attempt to remove the directory when done as this script is run in a
CI system where all state is thrown away on completion.
foolip added a commit that referenced this pull request Oct 22, 2019
In #19580 retry for
manifest generation was introduced, to make it more robust in case of
network errors or the GitHub search API not indexing the merged PR
fast enough.

However, an accidental commit to master showed that this won't work:
https://github.com/web-platform-tests/wpt/runs/265191690

It first failed with "No PR found for 9e1d0a2"
because there was no PR, just a direct push to master. Retries then
failed with "File exists: '/home/runner/meta'".

Use a new temporary directory every time to deal with this. Don't
attempt to remove the directory when done as this script is run in a
CI system where all state is thrown away on completion.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 31, 2019
…st generation in CI, a=testonly

Automatic update from web-platform-tests
Use a new temporary directory for manifest generation in CI (#19802)

In web-platform-tests/wpt#19580 retry for
manifest generation was introduced, to make it more robust in case of
network errors or the GitHub search API not indexing the merged PR
fast enough.

However, an accidental commit to master showed that this won't work:
https://github.com/web-platform-tests/wpt/runs/265191690

It first failed with "No PR found for 9e1d0a26f155ccb088d342b0036e535d033423c1"
because there was no PR, just a direct push to master. Retries then
failed with "File exists: '/home/runner/meta'".

Use a new temporary directory every time to deal with this. Don't
attempt to remove the directory when done as this script is run in a
CI system where all state is thrown away on completion.
--

wpt-commits: 3b13e3a1313e08cf4bc5270ab59c149a2bc56260
wpt-pr: 19802
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Oct 31, 2019
…st generation in CI, a=testonly

Automatic update from web-platform-tests
Use a new temporary directory for manifest generation in CI (#19802)

In web-platform-tests/wpt#19580 retry for
manifest generation was introduced, to make it more robust in case of
network errors or the GitHub search API not indexing the merged PR
fast enough.

However, an accidental commit to master showed that this won't work:
https://github.com/web-platform-tests/wpt/runs/265191690

It first failed with "No PR found for 9e1d0a26f155ccb088d342b0036e535d033423c1"
because there was no PR, just a direct push to master. Retries then
failed with "File exists: '/home/runner/meta'".

Use a new temporary directory every time to deal with this. Don't
attempt to remove the directory when done as this script is run in a
CI system where all state is thrown away on completion.
--

wpt-commits: 3b13e3a1313e08cf4bc5270ab59c149a2bc56260
wpt-pr: 19802
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Nov 1, 2019
…st generation in CI, a=testonly

Automatic update from web-platform-tests
Use a new temporary directory for manifest generation in CI (#19802)

In web-platform-tests/wpt#19580 retry for
manifest generation was introduced, to make it more robust in case of
network errors or the GitHub search API not indexing the merged PR
fast enough.

However, an accidental commit to master showed that this won't work:
https://github.com/web-platform-tests/wpt/runs/265191690

It first failed with "No PR found for 9e1d0a26f155ccb088d342b0036e535d033423c1"
because there was no PR, just a direct push to master. Retries then
failed with "File exists: '/home/runner/meta'".

Use a new temporary directory every time to deal with this. Don't
attempt to remove the directory when done as this script is run in a
CI system where all state is thrown away on completion.
--

wpt-commits: 3b13e3a1313e08cf4bc5270ab59c149a2bc56260
wpt-pr: 19802

UltraBlame original commit: 9fb5eab0466068bf41f77823582dde61fa26ed7b
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Nov 1, 2019
…st generation in CI, a=testonly

Automatic update from web-platform-tests
Use a new temporary directory for manifest generation in CI (#19802)

In web-platform-tests/wpt#19580 retry for
manifest generation was introduced, to make it more robust in case of
network errors or the GitHub search API not indexing the merged PR
fast enough.

However, an accidental commit to master showed that this won't work:
https://github.com/web-platform-tests/wpt/runs/265191690

It first failed with "No PR found for 9e1d0a26f155ccb088d342b0036e535d033423c1"
because there was no PR, just a direct push to master. Retries then
failed with "File exists: '/home/runner/meta'".

Use a new temporary directory every time to deal with this. Don't
attempt to remove the directory when done as this script is run in a
CI system where all state is thrown away on completion.
--

wpt-commits: 3b13e3a1313e08cf4bc5270ab59c149a2bc56260
wpt-pr: 19802

UltraBlame original commit: 9fb5eab0466068bf41f77823582dde61fa26ed7b
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Nov 1, 2019
…st generation in CI, a=testonly

Automatic update from web-platform-tests
Use a new temporary directory for manifest generation in CI (#19802)

In web-platform-tests/wpt#19580 retry for
manifest generation was introduced, to make it more robust in case of
network errors or the GitHub search API not indexing the merged PR
fast enough.

However, an accidental commit to master showed that this won't work:
https://github.com/web-platform-tests/wpt/runs/265191690

It first failed with "No PR found for 9e1d0a26f155ccb088d342b0036e535d033423c1"
because there was no PR, just a direct push to master. Retries then
failed with "File exists: '/home/runner/meta'".

Use a new temporary directory every time to deal with this. Don't
attempt to remove the directory when done as this script is run in a
CI system where all state is thrown away on completion.
--

wpt-commits: 3b13e3a1313e08cf4bc5270ab59c149a2bc56260
wpt-pr: 19802

UltraBlame original commit: 9fb5eab0466068bf41f77823582dde61fa26ed7b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

manifest-build-and-tag spends most time in Docker build step
6 participants