Skip to content

Git Patch BoringSSL to disable Windows build warnings/errors #454

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

Merged
merged 13 commits into from
Jun 9, 2021

Conversation

DellaBitta
Copy link
Contributor

@DellaBitta DellaBitta commented Jun 9, 2021

This change fixes the boringSSL sources so that they no longer causes warnings on windows-latest runners.

  • Switched the boringSSL dependency to a git checkout instead of a tarball download.
  • Created a git patch to alter boringSSL's cmake files, adding C4255 as a warning that shall be suppressed.
  • Updated GHA configuration to use windows-latest again. This was previously changed to windows-2016 since those tools didn't report C4255 as a warning. See Fix BoringSSL Windows CI targets failing with windows-latest #444 for more information about these reverted changes.

Other tests:

@google-cla google-cla bot added the cla: yes label Jun 9, 2021
@DellaBitta DellaBitta added the tests-requested: quick Trigger a quick set of integration tests. label Jun 9, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jun 9, 2021
@DellaBitta DellaBitta changed the title Git Patch BoringSSL dependency to disable build warnings/errors Git Patch BoringSSL to disable Windows build warnings/errors Jun 9, 2021
@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Jun 9, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 9, 2021
@DellaBitta DellaBitta added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Jun 9, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jun 9, 2021
@DellaBitta DellaBitta requested review from jonsimantov and vimanyu June 9, 2021 19:29
Copy link
Contributor

@vimanyu vimanyu left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Jun 9, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 9, 2021
@firebase firebase deleted a comment from github-actions bot Jun 9, 2021
@DellaBitta DellaBitta removed the tests: failed This PR's integration tests failed. label Jun 9, 2021
@DellaBitta
Copy link
Contributor Author

Tests failed during a download of the cloud sdk and not in any of the build steps. Packaging flow was fully succesful.

@DellaBitta DellaBitta merged commit 0d36667 into main Jun 9, 2021
@DellaBitta DellaBitta deleted the feature/git_patch_boringssl branch June 9, 2021 20:13
DellaBitta added a commit that referenced this pull request Jun 15, 2021
Windows BoringSSL builds are now larger due to #454. The checkout & build partition **(D:)** hits capacity when attempting to install the Google Cloud SDK, which is what we use to upload the final integraion_test build artifacts to storage.

This change moves the arhives destined for GCS to the **C:** drive, then installs the Cloud SDK after they're off of **D:**.  The migration of the integration tests only occurs on Windows, for now.

Some notes:
- We can’t set the build_testapps.py’s  --output_directory flag to directly output to the **C:** directory because python’s relpath operations fail when working across different drives.
- The **github.workspace** env var can be changed to override check-out & installation paths, but the [GHA team notes potential unknown side effects](https://github.community/t/is-it-possible-to-change-github-workspace/136094/3). Therefore I didn’t test this.
- We couldn't  use **mv** the results to **C:** as Firestore's build results include symlinks.  Using **tar** to ferry the files instead.
@firebase firebase locked and limited conversation to collaborators Jul 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants