[Internal] Refactoring how we integrate with dockerfile #13027

kaos · 2021-09-28T09:45:39Z

…ary. pantsbuild#13026 [ci skip-rust] [ci skip-build-wheels]

kaos · 2021-09-28T09:46:49Z

Seems like I have a bit of bootstrap issue with introducing a new tool lock file, as I get this error when I run generate_all_lockfiles.sh:

FileNotFoundError: [Errno 2] No such file or directory: '/Users/x/src/github/kaos/pants/src/python/pants/backend/docker/dockerfile_lockfile.txt'

@Eric-Arellano perhaps you can spot what I've missed...

I tried to touch the file, but then it complained of a missing header, naturally..

EDIT: solved, now making progress again...

Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

This reverts commit 4a87ea4. [ci skip-rust] [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano · 2021-09-29T20:12:59Z

src/python/pants/backend/docker/dockerfile_lockfile.txt

@@ -6,7 +6,7 @@
 # {
 #   "version": 2,
 #   "valid_for_interpreter_constraints": [
-#     "CPython>=3.6.1"
+#     "CPython>=3.7"


Why this change?

Ah, because dockerfile_parser.py is run with the PEX as its entrypoint, and thus was failing with Py36 because Py37 only features?

I think that's reasonable for us to only allow running with Py37+ given that Pants itself requires Py37, so it should be discoverable in the repo.

Feels like a losing battle to instead try to enforce that dockerfile_parser.py is Py36+ rather than Py37+.

Yes exactly. I tried it first, when it was just typing issues, but the dataclass..

Eric-Arellano

dockerfile_parser.py is a bit confusing in looking like parser.py. Maybe parser_wrapper_script.py?

Eric-Arellano · 2021-09-29T20:17:49Z

src/python/pants/backend/docker/dockerfile_lockfile.txt

@@ -6,7 +6,7 @@
 # {
 #   "version": 2,
 #   "valid_for_interpreter_constraints": [
-#     "CPython>=3.6.1"
+#     "CPython>=3.7"


Ah, because dockerfile_parser.py is run with the PEX as its entrypoint, and thus was failing with Py36 because Py37 only features?

I think that's reasonable for us to only allow running with Py37+ given that Pants itself requires Py37, so it should be discoverable in the repo.

Feels like a losing battle to instead try to enforce that dockerfile_parser.py is Py36+ rather than Py37+.

kaos · 2021-09-29T20:59:56Z

Yeah, I was considering pushing the lot down a level too, so it would be ..backend.docker.parser.[parser|tool|...]
Just straw names there.. would like to get it into a workable state first though.

Does the rest make kind of sense?

kaos · 2021-09-29T21:01:07Z

Sorry, just spotted there are review comments. The ui aren't the best on the phone..

Eric-Arellano

Yes, this PR indeed looks great overall! Thank you for the change.

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano

Thanks! 🙌

* [internal] Run pyupgrade on src/python/pants/backend/python ([#13073](#13073)) * [internal] Re-enable some skipped JVM tests. ([#13074](#13074)) * [internal] Use `DownloadedExternalModules` when analyzing external Go packages ([#13076](#13076)) * [internal] Use `DownloadedExternalModules` during Go target generation ([#13070](#13070)) * [internal] Replace deprecated use of `[pytest] junit_xml_dir` with `[test] xml_dir. ([#13069](#13069)) * [internal] Add `DownloadedExternalModules` for Go ([#13068](#13068)) * [internal] Always use jars on the user classpath, and generalize transitive classpath building ([#13061](#13061)) * Add failing tests for Go external modules ([#13065](#13065)) * [internal] java: fix version in test ([#13064](#13064)) * [internal] Skip additional inference tests ([#13062](#13062)) * [internal] java: enable cycles for file-level targets generated by `java_sources` ([#13058](#13058)) * [internal] Add a `@logging` decorator for tests. ([#13060](#13060)) * [internal] Improve compatibility of nailgun with append only caches, and use them for Coursier ([#13046](#13046)) * [internal] Stop using `go.sum` when generating `_go_external_package` targets ([#13052](#13052)) * [internal] Rename `go_module` target to `go_mod` ([#13053](#13053)) * [internal] Refactor `go/util_rules/external_module.py` ([#13051](#13051)) * [internal] go: add analyzer and rules for test sources ([#13041](#13041)) * [Internal] Refactoring how we integrate with dockerfile ([#13027](#13027)) * [internal] Simplify `go/package_binary.py` ([#13045](#13045)) * [internal] Refactor `OwningGoMod` ([#13042](#13042)) * [internal] Refactor `go_mod.py` ([#13039](#13039)) * [internal] Record metadata on engine-aware params ([#13040](#13040)) * [internal] Test discovery of `go` binary ([#13038](#13038)) * [internal] Extract directory setup for terraform linters / formatters into a separate rule. ([#13037](#13037)) * [internal] java: register dependency inference rules ([#13035](#13035)) * [internal] Add `strutil.bullet_list()` to DRY formatting ([#13031](#13031)) * Minor cleanups for the autoflake linter / formatter. ([#13032](#13032)) * Ensure XML results recorded for both pytest and junit ([#13025](#13025)) * [internal] go: refactor compilation into separate rule ([#13019](#13019)) * [internal] go: refactor link step into separate rule ([#13022](#13022)) * [internal] go: enable plugin in repo and cleanup test project ([#13018](#13018)) * [internal] go: use colon to separate binary name and version ([#13020](#13020)) * [internal] tweak formatting of help text for sourcefile-validation subsystem. ([#13016](#13016)) * [internal] Use system-installed Go rather than installing via Pants ([#13007](#13007)) * Move the `process-execution-local-cleanup` hint to a more specific location. ([#13013](#13013)) * [internal] Split shell targets into atom vs generator ([#12957](#12957)) * Install Go in CI ([#13011](#13011)) * Refresh maintainers list. ([#13004](#13004)) * [internal] Refactor setup of GOROOT and `import_analysis.py` ([#13000](#13000)) * Infer dependencies on COPY of pex binaries for `docker_image`s. ([#12920](#12920)) * Prepare 2.7.0. ([#12995](#12995)) * [internal] jvm: skip JDK tests unless env var set ([#12994](#12994)) * [internal] jvm: limit caching of JDK setup processes ([#12992](#12992)) * [internal] Async-ify `NailgunPool::connect` and `nailgun::CommandRunner`. ([#12990](#12990)) * [internal] Replace `java_library` with `java_source` and `java_sources`, and add `java_test` ([#12976](#12976)) * Prepare 2.7.0rc5. ([#12987](#12987)) * [internal] terraform: refactor parser script into its own file ([#12985](#12985)) * [internal] jvm/java: ensure JDK downloaded in one process ([#12972](#12972)) * add JDK to GitHub Actions CI ([#12977](#12977)) * [internal] Re-enable the `clippy::used_underscore_binding` check. ([#12983](#12983)) * [internal] Use target generation for `_go_external_package` ([#12929](#12929)) * [internal] Bump CI token expiration threshold. ([#12974](#12974)) * [internal] Re-enable the Java backend. ([#12971](#12971)) * [internal] Implement `@union`s via `Query`s ([#12966](#12966)) * Remove `Enriched*Result` classes in favor of `EngineAwareReturnType.cacheable` ([#12970](#12970)) * [internal] Remove spurious `python_tests` directive ([#12968](#12968)) * [internal] Python coverage report generation uses precomputed addresses. ([#12965](#12965)) * Add PackageRootedDependencyMap for mapping inferred Java dependencies ([#12964](#12964))

[Internal] WIP: Refactoring how we integrate with the dockerfile libr…

a7ac76b

…ary. pantsbuild#13026 [ci skip-rust] [ci skip-build-wheels]

kaos requested a review from Eric-Arellano September 28, 2021 09:52

kaos marked this pull request as ready for review September 29, 2021 09:04

kaos changed the title ~~[Internal] WIP: Refactoring how we integrate with dockerfile~~ [Internal]Refactoring how we integrate with dockerfile Sep 29, 2021

kaos changed the title ~~[Internal]Refactoring how we integrate with dockerfile~~ [Internal] Refactoring how we integrate with dockerfile Sep 29, 2021

kaos added 3 commits September 29, 2021 13:19

Fix type hints to work for py>=3.6.1.

4a87ea4

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Revert "Fix type hints to work for py>=3.6.1."

bda65db

This reverts commit 4a87ea4. [ci skip-rust] [ci skip-build-wheels]

Require py >=3.7 to run dockerfile parser.

39e5b5f

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano reviewed Sep 29, 2021

View reviewed changes

Eric-Arellano reviewed Sep 30, 2021

View reviewed changes

kaos added 3 commits September 30, 2021 09:07

Docker backend now requires the pex rules to work.

b8fcf8c

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Rename files for clarity.

3dd2328

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

missed one rename.

9dac161

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano approved these changes Sep 30, 2021

View reviewed changes

kaos merged commit 7adc235 into pantsbuild:main Sep 30, 2021

kaos deleted the issue/13026_dockerfile_tool branch September 30, 2021 15:03

wisechengyi mentioned this pull request Oct 2, 2021

2.8.0.dev3 prep #13081

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Internal] Refactoring how we integrate with dockerfile #13027

[Internal] Refactoring how we integrate with dockerfile #13027

kaos commented Sep 28, 2021 •

edited

Loading

kaos commented Sep 28, 2021 •

edited

Loading

Eric-Arellano Sep 29, 2021

Eric-Arellano Sep 29, 2021

kaos Sep 29, 2021

Eric-Arellano left a comment

Eric-Arellano Sep 29, 2021

kaos commented Sep 29, 2021

kaos commented Sep 29, 2021

Eric-Arellano left a comment

Eric-Arellano left a comment

[Internal] Refactoring how we integrate with dockerfile #13027

[Internal] Refactoring how we integrate with dockerfile #13027

Conversation

kaos commented Sep 28, 2021 • edited Loading

kaos commented Sep 28, 2021 • edited Loading

Eric-Arellano Sep 29, 2021

Choose a reason for hiding this comment

Eric-Arellano Sep 29, 2021

Choose a reason for hiding this comment

kaos Sep 29, 2021

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Sep 29, 2021

Choose a reason for hiding this comment

kaos commented Sep 29, 2021

kaos commented Sep 29, 2021

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

kaos commented Sep 28, 2021 •

edited

Loading

kaos commented Sep 28, 2021 •

edited

Loading