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

Automate v8 source upgrade #181

Merged

Conversation

GustavoCaso
Copy link
Contributor

@GustavoCaso GustavoCaso commented Sep 27, 2021

Related to #165

This PR adds two new workflows: upgrade and automatic build v8 binaries for macOS and Linux.

The upgrade workflow is triggered every first day of the month, and the automatic v8 build gets trigger on every pull_request which branch follows the v8_<version>_upgrade pattern.

The upgrade workflow runs every day and check the current stable v8 version with the currently installed version on the repo.

For that, I created a v8_version file inside the deps folder and a python script to check the versions. If the versions are the same the script returns 1 stopping the workflow from continuing.

This would allow us to have a more robust upgrade flow.

Until we figured out how to fully automate the Windows build the new automatic build v8 workflow only takes care of building the binaries for macOS and Linux. Example build that failed for Windows.

@genevieve @rogchap @dylanahsmith Please me know what you think.

@GustavoCaso GustavoCaso force-pushed the automate-v8-build-for-macosx-and-linux branch from 556c3b2 to 6b70808 Compare September 27, 2021 11:05
@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

Merging #181 (256a7c7) into master (6dff945) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #181   +/-   ##
=======================================
  Coverage   95.59%   95.59%           
=======================================
  Files          16       16           
  Lines         545      545           
=======================================
  Hits          521      521           
  Misses         15       15           
  Partials        9        9           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6dff945...256a7c7. Read the comment docs.

@GustavoCaso GustavoCaso force-pushed the automate-v8-build-for-macosx-and-linux branch from eee259c to d57201c Compare September 28, 2021 11:14
- Created an scheduled workflow that runs every month, which
automated the steps to upgrade the v8 binaries from source
https://github.com/rogchap/v8go#upgrading-the-v8-binaries

- Created a new workflow that is triggered by a pull_request
which build the v8 binaries for MacOs and Linux. Window
is excluded until we figured out a way to solve the compiler
issues.
@GustavoCaso GustavoCaso force-pushed the automate-v8-build-for-macosx-and-linux branch from d57201c to 945bd17 Compare September 28, 2021 11:15
@GustavoCaso
Copy link
Contributor Author

GustavoCaso commented Sep 29, 2021

@rogchap would you be able to have a look? Thanks 😄


on:
schedule:
- cron: '0 0 * * *' # Run every day
Copy link
Owner

Choose a reason for hiding this comment

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

Stable releases are on average every 4 weeks. Running every day seems excessive, but I guess most of the time it will just be a version check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Roughly every 4 weeks a new major Stable release is done.

(Source: https://v8.dev/docs/release-process#stable-releases)

Emphasis on "major" was mine, they more frequently release patch version bumps.

Currently, https://omahaproxy.appspot.com shows the most recent version bump was only about a week apart:

  • current_reldate: 09/30/21
  • previous_reldate: 09/24/21

If we tracked the branch head for the stable channel (currently branch-heads/9.4) then that would make the changes even more frequent. Releasing fixes that frequently might be overkill, but automating the preparation work for getting a security fix out would be useful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can start with every day and if it gets noisy (which it shouldn't if it's largely version checks and silent exiting if releases are more like once a week), we can reduce the frequency.

@GustavoCaso GustavoCaso force-pushed the automate-v8-build-for-macosx-and-linux branch 3 times, most recently from 6d25003 to 0946f7a Compare October 14, 2021 17:26
@GustavoCaso GustavoCaso force-pushed the automate-v8-build-for-macosx-and-linux branch from 0946f7a to 7db1d65 Compare October 14, 2021 17:28
deps/upgrade_v8.py Outdated Show resolved Hide resolved
deps/upgrade_v8.py Outdated Show resolved Hide resolved
deps/upgrade_v8.py Outdated Show resolved Hide resolved
deps/upgrade_v8.py Outdated Show resolved Hide resolved
Comment on lines 129 to 132
# fetch latest v8
subprocess.run(["fetch", "v8"],
cwd=v8_path,
env=env)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The v8 source will already be checked out as a git submodule, so I'm not sure if this step is relevant anymore. Also, if it were used, it would be used from deps_path, otherwise it would create another v8 checkout directory under v8_path which we don't want.

For existing git checkouts, gclient sync would be used to fetch the dependencies, as is done in deps/build.py. Actually, I think the only reason why we might want to do that here is to update the depot_tools commit that is being used. Otherwise, this script is just using the v8 git checkout (to copy the include headers).

deps/upgrade_v8.py Outdated Show resolved Hide resolved
deps/upgrade_v8.py Outdated Show resolved Hide resolved
@GustavoCaso
Copy link
Contributor Author

@genevieve Thanks for 🟢, but I'm experimenting with my own fork and there are some things that need to be addressed to be able to merge into master. Give me a little more time and I will let you know when is a good time to merge it

@GustavoCaso GustavoCaso force-pushed the automate-v8-build-for-macosx-and-linux branch from 1d390ce to 5e0efdb Compare October 21, 2021 20:21
@GustavoCaso
Copy link
Contributor Author

@rogchap @dylanahsmith @genevieve I've done some explorations on my fork and this is the conclusion I was able to gether.

  • The new workflow v8upgrade.yml works as expected
  • The branch created by the workflow won't trigger any flows neither the build not the test ones. This is not an issue but rather a design from the GitHub actions docs

If you would like to trigger a workflow from a workflow run, you can trigger the event using a personal access token. You'll need to create a personal access token and store it as a secret. To minimize your GitHub Actions usage costs, ensure that you don't create recursive or unintended workflow runs. For more information on storing a personal access token as a secret, see "Creating and storing encrypted secrets."

  • I tested on my fork by creating a PAT and it worked, but with the problem that the test workflow would also be triggered. This is unfortunate because they would always fail and it is a waste of time.
    Possible solution: Would be to create a new base branch maybe v8_upgrades and make sure that the test workflow is not triggered on pull request or push which the target is that branch.

Since these changes I believe are out of the scope of this PR, I think we can merge this one to start getting the benefits of the upgrade workflow and in the future, we can try improving it.
Also, once this hits master I'm happy to continue working on improving the workflow.

@GustavoCaso
Copy link
Contributor Author

I validated that PR created with the v8upgrade workflow will not be overwrite by scheduled runs.

In the case the workflows runs for the latest version and it takes some time to make the necessary changes for the different distributions, the workflow will be trigger everyday but since the changes to the PR are identical the workflow will exit successfully. You can see an example on my fork actions https://github.com/GustavoCaso/v8go/actions/workflows/v8upgrade.yml

Copy link
Collaborator

@dylanahsmith dylanahsmith left a comment

Choose a reason for hiding this comment

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

A couple of minor suggestions, otherwise, LGTM

README.md Outdated
1) Optionally build the V8 binary for your OS: `cd deps && ./build.py`. V8 is a large project, and building the binary
can take up to 30 minutes. Once built all the tests should still pass via `go test -v .`
1) Commit your changes, making sure that the git submodules have been updated to the new checksum for the version of V8.
Make sure *NOT* to add your build of the binary, as this will be build via CI.
1) Because the build is so long, this is not automatically triggered. Go to the [V8
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1) Because the build is so long, this is not automatically triggered. Go to the [V8
1) The build is not yet triggered automatically. To trigger it manually, go to the [V8

with open(os.path.join(src_path, V8_VERSION_FILE), "w") as v8_file:
v8_file.write(version)

def read_v8_file(src_path):
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: I think mentioning that it is a version file would make the code clearer

Suggested change
def read_v8_file(src_path):
def read_v8_version_file(src_path):

same for update_v8_file

@GustavoCaso GustavoCaso force-pushed the automate-v8-build-for-macosx-and-linux branch from 749e3ab to 256a7c7 Compare October 22, 2021 16:11
@dylanahsmith dylanahsmith changed the title Automate v8 build for macosx and linux Automate v8 source upgrade Oct 22, 2021
@dylanahsmith dylanahsmith merged commit ed7ef69 into rogchap:master Oct 22, 2021
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.

4 participants