This repository has been archived by the owner on Oct 2, 2023. It is now read-only.
Replace dependency on gflags with argparse #1582
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This should no longer be necessary now that what is tested no longer uses gflags.
smukherj1
reviewed
Aug 7, 2020
There was a problem hiding this comment.
Thanks for this PR! I been regretting switching to pip for gflags as well. Looks like a test image from image_test.py has a digest mismatch:
FAILED: //container:image_test (Summary)
--
| /var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/execroot/io_bazel_rules_docker/bazel-out/k8-py2-fastbuild/testlogs/container/image_test/test.log
| /var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/execroot/io_bazel_rules_docker/bazel-out/k8-py2-fastbuild/testlogs/container/image_test/test_attempts/attempt_1.log
| /var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/execroot/io_bazel_rules_docker/bazel-out/k8-py2-fastbuild/testlogs/container/image_test/test_attempts/attempt_2.log
| (14:26:42) INFO: From Testing //container:image_test:
| ==================== Test output for //container:image_test:
| ............................................F......
| ======================================================================
| FAIL: test_with_passwd_tar (__main__.ImageTest)
| ----------------------------------------------------------------------
| Traceback (most recent call last):
| File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/853/execroot/io_bazel_rules_docker/bazel-out/k8-py2-fastbuild/bin/container/image_test.runfiles/io_bazel_rules_docker/container/image_test.py", line 435, in test_with_passwd_tar
| self.assertDigest(img, 'b7a3e3ea93db1cb8068aa1cbaf11f9b771c33eb4d220abe87fe140d1260b17d2')
| File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/853/execroot/io_bazel_rules_docker/bazel-out/k8-py2-fastbuild/bin/container/image_test.runfiles/io_bazel_rules_docker/container/image_test.py", line 70, in assertDigest
| self.assertEqual(img.digest(), 'sha256:' + digest)
| AssertionError: 'sha256:11ef1c5225ed570d11a9fc7c37a5959e91701318a023a6e754d50e34bd120929' != 'sha256:b7a3e3ea93db1cb8068aa1cbaf11f9b771c33eb4d220abe87fe140d1260b17d2'
The image in question is here. Please take a look!
Use action='append' instead of nargs='+' to allow accumulation of multiple flags.
|
Had a small mistranslation for replacing the multistring, should be fixed! |
smukherj1
approved these changes
Aug 7, 2020
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: robbertvanginkel, smukherj1 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/gcbrun |
|
Thanks! |
|
This is great :) Solved a regression for Wix when upgrading to bazel 3.5.0 |
Closed
apattidb
pushed a commit
to databricks/rules_docker
that referenced
this pull request
Aug 20, 2021
* Replace gflags with build in argparse * Remove pip_deps install from tests This should no longer be necessary now that what is tested no longer uses gflags. * Fix multistring in argparse Use action='append' instead of nargs='+' to allow accumulation of multiple flags.
apattidb
pushed a commit
to databricks/rules_docker
that referenced
this pull request
Aug 20, 2021
* Replace gflags with build in argparse * Remove pip_deps install from tests This should no longer be necessary now that what is tested no longer uses gflags. * Fix multistring in argparse Use action='append' instead of nargs='+' to allow accumulation of multiple flags.
apattidb
pushed a commit
to databricks/rules_docker
that referenced
this pull request
Oct 13, 2021
* Replace gflags with build in argparse * Remove pip_deps install from tests This should no longer be necessary now that what is tested no longer uses gflags. * Fix multistring in argparse Use action='append' instead of nargs='+' to allow accumulation of multiple flags.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since #1535, rules_docker depends on a global
@pip_depsexternal repo and pip to install the python gflags library. This easily conflicts with other python rules that claim that name (#1404) and forces a dependency on pypi, making rules_docker harder to depend upon. I'd prefer not to have to setup a pypi mirror just to be able to reliably use rules_docker.This seems pretty painful for importing a flag library, the comment in #1533 (comment) suggests
Which this PR attempts to implement.