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

Fix #2659: Add third party wrapper libs #2663

Merged
merged 6 commits into from
Feb 11, 2021

Conversation

BenHenning
Copy link
Member

Fix #2659.

This PR introduces a wrapper library for all third party dependencies for the following benefits:

  • Easy auditing of dependencies
  • Forced prevention of one-version violations (e.g. we can force only 1 version of a dependency to be included everywhere)
  • Ensure correct dependencies are being pulled in (I removed 1 duplicate & added some missing ones from just creating this PR)
  • Force certain dependencies to only be usable in tests (e.g. JUnit)
  • Allow correct buildifier sorting for deps lists (it doesn't like macros like artifact)
  • Keep all dependencies in build files to be within our repository which conceptually simplifies managing the deps lists for individual libraries
  • This is in-line with Bazel's suggested best practices: https://docs.bazel.build/versions/master/best-practices.html#third-party-dependencies & https://docs.bazel.build/versions/master/best-practices.html#versioning
  • Easier version upgrading for dependencies

This doesn't quite handle all cases (e.g. see WORKSPACE), but it handles most & is at least a step in the right direction.

This PR includes some unrelated fixes in format_import_proto_library.

I'm blocking this PR on #1904 since that actually fixes all test builds Bazel, and I can't be confident this PR is correct until I see those builds succeed & run in CI. I only verified //:oppia so far.

Note this might be specific to Bazel 4.0, but this seems more correct
anyway.
I'm probably just going to migrate everything since I think this can be
easily generalized.
Also, automatically generate the wrapper libraries. Only constraint
layout is being used currently.
This adds more versions to a central location for management. It's not
quite comprehensive, but it does provide at least a place for these
versions to live as the codebase continues to change over time.
Also, fixes some references to artifacts that weren't actually being
imported via Maven.
@BenHenning
Copy link
Member Author

/cc @fsharpasharp as well. Apparently you weren't added as a collaborator yet (just sent the invite), so I can't assign you yet. Once you comment I should be able to though.

Copy link
Contributor

@prayutsu prayutsu left a comment

Choose a reason for hiding this comment

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

The PR LGTM, just added some comments to get me more clearer How this can be useful for the "third-party" project.

Comment on lines -25 to +28
sed 's/import \"/import \"model\/src\/main\/proto\//g' |
sed 's/\"model\/src\/main\/proto\/exploration/\"model\/processed_src\/main\/proto\/exploration/g' |
sed 's/\"model\/src\/main\/proto\/topic/\"model\/processed_src\/main\/proto\/topic/g' |
sed 's/\"model\/src\/main\/proto\/question/\"model\/processed_src\/main\/proto\/question/g' > $@
sed 's/import \\"/import \\"model\\/src\\/main\\/proto\\//g' |
sed 's/\\"model\\/src\\/main\\/proto\\/exploration/\\"model\\/processed_src\\/main\\/proto\\/exploration/g' |
sed 's/\\"model\\/src\\/main\\/proto\\/topic/\\"model\\/processed_src\\/main\\/proto\\/topic/g' |
sed 's/\\"model\\/src\\/main\\/proto\\/question/\\"model\\/processed_src\\/main\\/proto\\/question/g' > $@
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for correcting this, probably it was causing an error while I was trying to run bazel query commands.

third_party/versions.bzl Show resolved Hide resolved
@prayutsu prayutsu removed their assignment Feb 9, 2021
@prayutsu
Copy link
Contributor

prayutsu commented Feb 9, 2021

@BenHenning Please mark the above conversation resolved on my behalf as I can not resolve them.

Copy link
Contributor

@jcqli jcqli left a comment

Choose a reason for hiding this comment

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

Very neat solution!

@jcqli jcqli removed their assignment Feb 9, 2021
Copy link
Contributor

@fsharpasharp fsharpasharp left a comment

Choose a reason for hiding this comment

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

The separation looks very clean!

@vinitamurthi vinitamurthi removed their assignment Feb 11, 2021
@vinitamurthi
Copy link
Contributor

Merging this since the comments seemed to be addressed

@vinitamurthi vinitamurthi merged commit f8f068d into develop Feb 11, 2021
@vinitamurthi vinitamurthi deleted the add-third-party-wrapper-libs branch February 11, 2021 03:31
@BenHenning
Copy link
Member Author

Thanks all!

BenHenning added a commit that referenced this pull request Feb 11, 2021
This fixes some tests that were broken after recent PRs, and fixed a
visibility error introduced in #2663.
BenHenning added a commit that referenced this pull request Feb 12, 2021
…Oppia tests (#1904)

* Update Bazel CI workflow

Build all Oppia targets in Bazel rather than just the binary target.

* Update main.yml

Reset the workflow name so that it doesn't need to be renamed in GitHub settings.

* Introduce remote caching in Bazel.

This uses a remote storage service with a local file encrypted using
git-secret to act as the authentication key for Bazel to read & write
artifacts to the service for caching purposes.

* Add debug line.

* Disable workflows + fix debug line.

* More debugging.

* More debugging.

* Work around GitHub hiding secret since we're debugging.

* Use base64 to properly encode newlines in GPG keys.

* Remove debug lines before changing back to correct GPG key.

* Switch to production key.

* Fix env variable reference. Lock-down actions workflows via codeowners.

* Install git-secret to default location.

* Add details. Debug $PATH.

* Fix pathing for git-secret.

* Dummy commit to re-trigger actions.

* Undo commit to see if this one shows up.

* Fix git-secret pathing try 2.

* New commit to re-trigger action.

* Path debugging.

* Workaround to get GitHub to show changes.

* Update runner to use Bash.

Reference:
https://github.community/t/github-path-does-not-add-to-the-path/143992.

* Restore binary-only build, other builds, and introduce new workflow for
building all Bazel targets.

* Remove debug lines.

* Rename & remove keep_going.

* Compute matrix containing all test targets.

This will allow us to distribute parallelization responsibilities partly
to GitHub actions to hopefully have slightly better throughput. See
https://github.blog/changelog/2020-04-15-github-actions-new-workflow-features/
for reference on how this mechanism works.

* Simplify, fix some issues, and debug instead of run.

* Turn on actual testing.

* Lower parallelization since GitHub started cancelling tasks.

* Try 15 parallel jobs instead.

* Turn off fail-fast instead of limiting parallelization.

Fail fast seems to be the reason why the tests aren't completing, not
quota (since if too many jobs are started, the extras should just be
queued until resources open up).

* Simplify workflow & allow it to be required.

Also, introduce bazelrc file to simplify the CI CLIs interacting with
Bazel.

* Add test change to investigate computing affected targets.

* Another test change to compute affected targets.

* Update workflow to use future script compute_affected_tests.sh.

Also, ignore Bazel directories in Git (to ease local development).

* Add script to compute affected targets.

This script is primarily meant to be used for CI.

* Execute tests in parallel to build.

This creates a new job to compute affected targets alongside the build.
This may result in the initial build being a bit slower, but subsequent
commits should be fast if remote caching is enabled. This will also
result in better performance for forks that can't use remote caching.

* Script clean-ups.

Also, re-trigger actions.

* Try to ensure develop branch is available for change analysis.

* Add automatic workflow cancellation.

Also, add support to explicitly run all tests if '[RunAllTests]' is in
the PR title.

* Attempt to make conditional matrix computation work.

* Remove join since it was used incorrectly.

* Add support for testing when Bazel, BUILD, and WORKSPACE files change.

One consequence is the current Bazel file structure is very tight, so
any changes will likely run the whole suite. This will get better over
time.

Also, show test logs for all test runs.

* Fix broken tests by adding missing dep library.

* Finalize PR.

1. Expand codeowners to include all workflow files.
2. Remove test comments in Kotlin files.
3. Re-enable all workflows.
4. Attempt to fix tests broken on actions but not locally by adding more
thread synchronization points.

* Lint fix.

* Fix timing issues and JDK 9-specific regression.

See comment thread in #1904 for more context.

* Restore workflow names + introduce test check.

The new test check workflow will be a static job that will be required
to pass. One failing test is being introduced to verify that this check
fails as expected.

The original workflow names are being restored so that they don't need
to be renamed in GitHub settings (since that would introduce a
discontinuity in CI service & require multiple migratiaon PRs to fix).

* Update StateFragmentLocalTest.kt

Remove fail-on-purpose test since it verified what it needed to: the new test status check job is working as expected.

* Address reviewer comments.

* Fix most tests broken in Bazel after syncing.

* Gitignore + fix broken test.

The test failure here only happens when using JDK9+ (which doesn't
happen in the Gradle world, or on CI).

The .gitignore is since we can't yet specify a .bazelproject in a
shareable way.

* Lint fixes.

* Post-merge clean-up.

* Fix broken post-merge test.

* Remove unnecessary codeowners per earlier codeowners setup.

* Fix
ItemSelectionInputDoesNotContainAtLeastOneOfRuleClassifierProviderTest.

* Disable image region selection tests.

These tests can't correctly pass on Robolectric until #1611 is fixed, so
disabling them for the time being to avoid the image loading flake
happening on CI (but not locally). Note that chances are a fix will
still be needed for the flake, but that can be addressed later.

* Disable 2 previously missed tests.

* Post-merge lint fix.

* Add missing dependency.

Verified all tests build & pass both for JDK 9 & 11. Hopefully they will
work as expected on CI.

* Add missing codeowners for new files.

* Post-merge fixes.

This fixes some tests that were broken after recent PRs, and fixed a
visibility error introduced in #2663.

* Move Bazel tests to new workflow.

This will make it easier to restart failures without having to also
restart unrelated tests.
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.

Migrate all third party dependencies to a dedicated top-level third_party folder
5 participants