docker: include file dependencies in docker build context properly. #12758

kaos · 2021-09-06T08:26:39Z

Turns out that there were quite a few corner cases uncovered wrgt targets, transitive targets and dependencies that I hadn't really come to terms with.

Signed-off-by: Andreas Stenius andreas.stenius@svenskaspel.se

[ci skip-rust]
[ci skip-build-wheels]

Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se>

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

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]

kaos

@benjyw I've codified your input wrgt how we want to treat file deps for docker images in a test case.

Turns out, it didn't quite work at all, as it were :P

Intend to follow up with more tests...

src/python/pants/backend/docker/docker_build_context.py

src/python/pants/backend/docker/docker_build_context_test.py

[ci skip-build-wheels]

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

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]

[ci skip-build-wheels]

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

kaos · 2021-09-07T09:19:06Z

OK, I think this is a good change to merge, once approved. @benjyw

kaos · 2021-09-07T18:41:19Z

@benjyw do you feel this PR solves the issues covered by #12694 ?

Edit: how did chewing on the context-name-calling go?

benjyw

Good cleanups! See my comments inline.

benjyw · 2021-09-08T08:50:20Z

src/python/pants/backend/docker/docker_build_context.py

-    # Get all sources, going into the build context.
+    # Get all dependencies from those root targets.
+    all_dependencies = await MultiGet(
+        Get(Targets, DependenciesRequest(target.get(Dependencies), include_special_cased_deps=True))


Why include_special_cased_deps=True? Was this on purpose, or copy-pasta? I don't actually think we want that. Special-cased deps are a weird thing, that possibly shouldn't exist, but right now they do things like make sure that if you depend on relocated_files you don't also depend on the original pre-relocation files.

Otherwise I didn't get files targets referenced from my dependencies.

hmm, well, guess it was something else, cause my tests works fine without. Will remove, then.

benjyw · 2021-09-08T08:51:20Z

src/python/pants/backend/docker/docker_build_context.py

+        for target in transitive_targets.roots
+    )
+
+    # Get Dockerfiles from all roots.


Note that there will be exactly one root, because transitive_targets was created from a single address.

Adjust comment?

Yeah, probably good to clarify for the casual reader.

benjyw · 2021-09-08T08:52:07Z

src/python/pants/backend/docker/docker_build_context.py

-            sources_fields=[t.get(Sources) for t in transitive_targets.closure],
-            for_sources_types=(DockerImageSources, FilesSources, ResourcesSources),
+            sources_fields=[t.get(Sources) for t in chain(*all_dependencies)],
+            for_sources_types=(FilesSources, ResourcesSources),


So this will now make sure that other Dockerfiles won't get pulled in via dependencies? Good!

I think we might as well go ahead and stop handling ResourcesSources here. As we discussed, it doesn't make much sense to do so.

Hmm, and we will still pull in the files() targets that we depend on via other docker_image targets, but as previously discussed this is a general problem that we don't need to solve here right now.

Check on dropping resources here.

No, I have tests covering that they don't pull in files from other docker_images.

What about archives?

I think those should just work - they are packaged resources. Are you seeing otherwise?

I'm surprised that files from other images (that this one depends on) aren't being pulled in. What prevents it?

I haven't tried anything with archives yet. Great if it just works. :)

all_dependencies are not really all dependencies for all transient targets, just all dependencies for the roots, i.e. our docker_file.

Ah, right. packaged resources, now I get it. Awesome. Yeah, that should work.

I rephrased the comments and renamed a few variables to make things hopefully clearer.

benjyw · 2021-09-08T09:27:30Z

src/python/pants/backend/docker/docker_build.py

@@ -71,7 +59,7 @@ async def build_docker_image(
        docker.build_image(
            field_set.image_tag,
            context.digest,
-            field_set.context_root,
+            ".",


Do we need this arg to build_image() at all now?

No, we could leave that as a default arg, and drop it here to make it cleaner. Will do.

I mean can we just get rid of it entirely in the signature of build_image, since we only set it once, to a hard-coded value. We can just hard-code it in the process call?

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]

benjyw

Looks good! I'm still unclear on why the files_B thing works, but good that it does, and I'll take a look at why some other time...

benjyw · 2021-09-08T13:38:44Z

src/python/pants/backend/docker/docker_build_context.py

+    )
+
+    # Get all dependencies for the root target.
+    root_dependencies = await MultiGet(


Ah I see, this will only take the direct dependencies. That's probably OK? It means that if there is some intermediate target (e.g., of type target() - which is a generic target useful for aggregating dependencies - we won't traverse it, but that's probably fine for now.

Ah, yes that's true.

benjyw · 2021-09-08T13:39:22Z

src/python/pants/backend/docker/docker_build_context.py

+    )
+
+    # Get all dependencies for the root target.
+    root_dependencies = await MultiGet(


Ah I see, this will only take the direct dependencies. That's probably OK? It means that if there is some intermediate target (e.g., of type target() - which is a generic target useful for aggregating dependencies) - we won't traverse it, but that's probably fine for now.

benjyw · 2021-09-08T13:39:47Z

src/python/pants/backend/docker/docker_build_context.py

+    )
+
+    # Get all dependencies for the root target.
+    root_dependencies = await MultiGet(


Ah I see, this will only take the direct dependencies. That's probably OK? It means that if there is some intermediate target (e.g., of type target() - which is a generic target useful for aggregating dependencies) - we won't traverse it, but that's probably fine for now.

benjyw

Great improvement!

benjyw · 2021-09-08T13:47:25Z

Looks good once CI passes!

kaos · 2021-09-08T14:26:40Z

I think the CI tests flunked.

[ci skip-rust] [ci skip-build-wheels]

…d#12758) Turns out that there were quite a few corner cases uncovered wrgt targets, transitive targets and dependencies that I hadn't really come to terms with. [ci skip-rust] [ci skip-build-wheels]

…k of #12758) (#12823) Turns out that there were quite a few corner cases uncovered wrgt targets, transitive targets and dependencies that I hadn't really come to terms with. [ci skip-rust] [ci skip-build-wheels] Co-authored-by: Andreas Stenius <andreas.stenius@svenskaspel.se>

…uild#12778)) * Add python-docx to the module mapping dictionary ([pantsbuild#12775](pantsbuild#12775)) * Add python-pptx to the module mapping dictionary ([pantsbuild#12776](pantsbuild#12776)) * Add `opencv-python` to the default Python module mapping ([pantsbuild#12777](pantsbuild#12777)) * Add `PyMuPDF` to the default Python module mapping ([pantsbuild#12774](pantsbuild#12774)) * Deprecate `--list-provides` option. ([pantsbuild#12759](pantsbuild#12759)) * Upgrade default `isort` to latest `isort==5.9.3` ([pantsbuild#12756](pantsbuild#12756)) * `OutputPathField.value_or_default()` no longer has an `Address` argument ([pantsbuild#12837](pantsbuild#12837)) * Properly include file dependencies in docker build context ([pantsbuild#12758](pantsbuild#12758)) * DigestSubset should not short-circuit when there are ignores involved. ([pantsbuild#12648](pantsbuild#12648)) * Improve cache reuse for `./pants package` when using a constraints file or lockfile ([pantsbuild#12807](pantsbuild#12807)) * Upgrade to Pex 2.1.48 and leverage packed layout. ([pantsbuild#12808](pantsbuild#12808)) * Fix backports of std lib modules like `dataclasses` not working with dependency inference ([pantsbuild#12818](pantsbuild#12818)) * Warn if `[python-repos]` is set during lockfile generation ([pantsbuild#12800](pantsbuild#12800)) * Add `version` to lockfile metadata headers ([pantsbuild#12788](pantsbuild#12788)) * jvm: add missing space to avoid two words running together ([pantsbuild#12793](pantsbuild#12793)) * Fix a markdown issue in a help string. ([pantsbuild#12766](pantsbuild#12766)) [ci skip-rust]

kaos added 3 commits August 25, 2021 13:24

digest subset should not short-circuit when there are ignores involved.

d7f345d

Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se>

fixes after review feedback.

4268459

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

kaos commented Sep 6, 2021

View reviewed changes

src/python/pants/backend/docker/docker_build_context.py Show resolved Hide resolved

src/python/pants/backend/docker/docker_build_context_test.py Show resolved Hide resolved

kaos requested a review from benjyw September 6, 2021 08:33

kaos added 6 commits September 6, 2021 15:46

Merge branch 'digest_subset_glob_exclude_bug' into docker/dep_tests

4889aca

[ci skip-build-wheels]

attempt at eliminating as much as possible the need for docker_image(…

efa2fa6

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

cleanup.

9648595

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]

indent.

3503d86

# 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]

Merge remote-tracking branch 'upstream/main' into docker/dep_tests

403537e

[ci skip-build-wheels]

docker: remove confusing context_root field of docker_image.

d768563

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

kaos added the needs-cherrypick label Sep 7, 2021

stuhood added this to the 2.7.x milestone Sep 7, 2021

benjyw reviewed Sep 8, 2021

View reviewed changes

kaos added 2 commits September 8, 2021 11:54

review feedback.

63c1355

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]

cleanup.

a9a1e7c

# 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]

benjyw approved these changes Sep 8, 2021

View reviewed changes

benjyw reviewed Sep 8, 2021

View reviewed changes

benjyw approved these changes Sep 8, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into docker/dep_tests

05eea4e

[ci skip-rust] [ci skip-build-wheels]

benjyw merged commit 001a1d6 into pantsbuild:main Sep 8, 2021

kaos deleted the docker/dep_tests branch September 9, 2021 05:04

stuhood removed the needs-cherrypick label Sep 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docker: include file dependencies in docker build context properly. #12758

docker: include file dependencies in docker build context properly. #12758

kaos commented Sep 6, 2021 •

edited

Loading

kaos left a comment •

edited

Loading

kaos commented Sep 7, 2021

kaos commented Sep 7, 2021 •

edited

Loading

benjyw left a comment

benjyw Sep 8, 2021

kaos Sep 8, 2021

kaos Sep 8, 2021

benjyw Sep 8, 2021

kaos Sep 8, 2021

benjyw Sep 8, 2021

benjyw Sep 8, 2021

benjyw Sep 8, 2021

benjyw Sep 8, 2021

kaos Sep 8, 2021

kaos Sep 8, 2021

benjyw Sep 8, 2021

kaos Sep 8, 2021

kaos Sep 8, 2021

kaos Sep 8, 2021

benjyw Sep 8, 2021

kaos Sep 8, 2021

benjyw Sep 8, 2021

kaos Sep 8, 2021

benjyw left a comment

benjyw Sep 8, 2021

kaos Sep 8, 2021

benjyw Sep 8, 2021

benjyw Sep 8, 2021

benjyw left a comment

benjyw commented Sep 8, 2021

kaos commented Sep 8, 2021

docker: include file dependencies in docker build context properly. #12758

docker: include file dependencies in docker build context properly. #12758

Conversation

kaos commented Sep 6, 2021 • edited Loading

kaos left a comment • edited Loading

Choose a reason for hiding this comment

kaos commented Sep 7, 2021

kaos commented Sep 7, 2021 • edited Loading

benjyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw left a comment

Choose a reason for hiding this comment

benjyw commented Sep 8, 2021

kaos commented Sep 8, 2021

kaos commented Sep 6, 2021 •

edited

Loading

kaos left a comment •

edited

Loading

kaos commented Sep 7, 2021 •

edited

Loading