build: reuse `parse.ContainerIgnoreFile` from buildah #26216

flouthoc · 2025-05-27T16:53:03Z

podman's logic to parse excludes from --ignorefile is not consistent with buildah, use code directly from buildah.

Does this PR introduce a user-facing change?

build: behavior of --ignorefile is consistent with buildah

Luap99 · 2025-05-27T17:13:44Z

cmd/podman/common/build.go

@@ -585,7 +585,7 @@ func buildFlagsWrapperToOptions(c *cobra.Command, contextDir string, flags *Buil
 	}

 	if flags.IgnoreFile != "" {
-		excludes, err := parseDockerignore(flags.IgnoreFile)
+		excludes, _, err := parse.ContainerIgnoreFile(contextDir, flags.IgnoreFile, containerFiles)


The function comment says

Deprecated since this might become internal only, please avoid relying on this function.

So what is the status with that, either we should not use it or change the buildah comment to actual commit to that as API? Personally I am all for such code sharing.
cc @nalind

We added notice here containers/buildah#4239 but at that time this function was not being used anywhere else, I think podman has a use-case for this. I am wondering if we can remove the deprecation notice ?

Right I agree. Just pointing out that the buildah comment should then get updated if we start to depend on it here.

Luap99

LGTM

openshift-ci · 2025-05-28T14:36:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, Luap99

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:

~~OWNERS~~ [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

flouthoc · 2025-05-30T15:10:37Z

@containers/podman-maintainers PTAL

mtrmac

I agree that the parser should be shared.

Right now, Buildah seems to me to be fairly uncertain about layering / responsibilities, and that leads to clearly unnecessary manipulation of containerFiles here.

Can the whole problem space be cleaned up further, please?

Maybe both Podman and Buildah should just pass the IgnoreFile option down to the executors, without parsing files at the CLI layer. (Purely aesthetically, I’d like that.)
Maybe buildah should export a special-case ParseIgnoreFile function that does not require the caller to provide containerFiles, and to only invoke the larger logic on ContainerIgnoreFile in some special situations (??). (Or invoke imagebuilder.ParseIgnore directly without a Buildah re-export?)
Quite likely I’m underappreciating the complexity of the design and completely misinterpreting the larger situation.

mtrmac · 2025-05-30T17:57:54Z

cmd/podman/common/build.go

@@ -585,7 +585,7 @@ func buildFlagsWrapperToOptions(c *cobra.Command, contextDir string, flags *Buil
 	}

 	if flags.IgnoreFile != "" {
-		excludes, err := parseDockerignore(flags.IgnoreFile)
+		excludes, _, err := parse.ContainerIgnoreFile(contextDir, flags.IgnoreFile, containerFiles)


if flags.IgnoreFile != "", containerFiles are not processed at all.

Meanwhile, imagebuildah.newExecutor also calls ContainerIgnoreFile, at that lower level.

I read BuildOptions.Excludes vs. BuildOptions.IgnoreFile to mean that it makes sense to only set one of the two. But buildah/pkg/cli.GenBuildOptions (not called in Podman, to be fair) sets both (computing Excludes from IgnoreFile).

What is the intended layering / responsibility division between the CLI and the underlying build code? (More of a Buildah question, not a Podman question, sure.)

flouthoc · 2025-05-30T18:53:37Z

@mtrmac I think we need to do parsing of ignorefile and containerfile at CLI level and not directly send it to the backend executor because in remote setup we need to prepare context and send it to backend. It is beneficial to do parsing of these at CLI level so we can send only appropriate files in the context and ignore sending files which are not needed.

Irrespective of the above case I still think somethings can be fixed at buildah at first and then podman can start using it, do you think it will be okay to those changes in a new PR and scope this PR only to the bug fix above ?

mtrmac · 2025-05-30T19:07:56Z

@mtrmac I think we need to do parsing of ignorefile and containerfile at CLI level and not directly send it to the backend executor because in remote setup we need to prepare context and send it to backend.

Thanks, so I was missing something.

do you think it will be okay to those changes in a new PR and scope this PR only to the bug fix above ?

What about those {Docker.Container}file.{docker,container}ignore and $buildContext/.{docker,container}ignore files, then? Do they need to be parsed at the CLI, or is that fine to do later because they will be transferred along the whole build context directory?

If they need to be parsed at the CLI, that’s not happening now.
If they will be transferred and parsed later, I’d prefer the Podman implementation, now, to be simplified: I.e. add something like parse.IgnoreFileAtPath with only one parameter to Buildah (and use it in Buildah’s pkg/cli/build.go), and then make a smaller change in Podman.

The more comprehensive cleanup of Buildah (if that still makes sense) can be done later, that’s pre-existing; but adding extra complexity to Podman would be adding to tech debt and is fairly cheaply avoidable now, when we understand the situation.

mtrmac · 2025-05-30T19:19:01Z

If they need to be parsed at the CLI, that’s not happening now.

Looking a bit more closely, $contextDir is transferred to the remote, but if containerFiles are not in $contextDir, ${containerFile}.{docker,container}ignore files are not currently transferred to the remote?! So that’s currently not working correctly. (But it’s pre-existing and not the bug this PR intends to fix.)

Hum… should the if flags.IgnoreFile != "" be removed in Podman, then? But that would turn this from a rare code path to an always-executing one, so we should be really sure that this is consistent with current Buildah behavior.

Or, maybe, keep the parse.ContainerIgnoreFile call as is in the PR now, and add a detailed comment about how this does not do what is necessary — under the theory that we will need to use containerFiles in the proper fix in the future, so this PR moves us somewhat in the right direction, even if the uses of containerFiles are currently misleading.

flouthoc · 2025-05-31T19:27:41Z

Looking a bit more closely, $contextDir is transferred to the remote, but if containerFiles are not in $contextDir, ${containerFile}.{docker,container}ignore files are not currently transferred to the remote?! So that’s currently not working correctly

I did not understand this comment exactly. As per function parse.ContainerIgnoreFile

If path is directly provided, it parses the ignorefile and returns the list of excludes.
Second it checks if ignorefiles exists on the same path as Contailerfiles, if it exists it parses the ignore file and returns the list of file which we want to exclude from the buildcontext.
If ignorefiles are not found in the same path as Containerfiles it looks for ignore files in $contextDir and if found it parses them and return list of excludes.

TLDR: It searches for ignorefiles in following order, then if found parses and returns list of excludes.

* The explicitly provided path (if any)
* For each file in containerFiles:
     -  <containerfile>.containerignore
     -  <containerfile>.dockerignore
* .containerignore in the context directory
* .dockerignore in the context directory

The function overall appears to me doing correctly and I did not understand why the use of contaerfiles is misleading ?

mtrmac · 2025-06-02T15:27:47Z

Looking a bit more closely, $contextDir is transferred to the remote, but if containerFiles are not in contextDir
${containerFile}.{docker,container}ignore files are not currently transferred to the remote?! So that’s currently not working correctly

I did not understand this comment exactly. As per function parse.ContainerIgnoreFile

If path is directly provided, it parses the ignorefile and returns the list of excludes.

Second

Look at the current call site: it calls parse.ContainerIgnoreFile iff flags.IgnoreFile is set, and in that case the other lookups don’t happen.

The function overall appears to me doing correctly and I did not understand why the use of contaerfiles is misleading ?

The function is (to my limited knowledge) fine; the misleading part is that the caller passes containerFiles in a situation where they are guaranteed to be ignored.

packit-as-a-service · 2025-06-03T15:29:52Z

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

mtrmac

LGTM (but I’d really prefer a careful second review by someone more familiar with this).

podman's logic to parse excludes from `--ignorefile` is not consistent with buildah, use code directly from imagebuilder. Closes: containers#25746 Signed-off-by: flouthoc <flouthoc.git@gmail.com>

flouthoc · 2025-06-03T19:05:54Z

Okay it seems like I cannot just remove flags.IgnoreFile and process all excludes earlier because it changes expected error messages for bud tests.

I have decided to use imagebuilder.ParseIgnore which is public and being used by the part of parse.ContainerIgnoreFile which makes following patch much more acceptable.

I will refactor things on buildah side and see if we can unify this part with buildah ( unrelated to this bug ).

mtrmac · 2025-06-03T19:09:05Z

LGTM, that’s a clear local improvement.

flouthoc · 2025-06-03T20:33:56Z

@containers/podman-maintainers PTAL

rhatdan · 2025-06-03T23:55:42Z

/lgtm

openshift-ci bot added the release-note label May 27, 2025

flouthoc mentioned this pull request May 27, 2025

podman build does not properly ignore absolute paths in .containerignore files. #25746

Closed

flouthoc force-pushed the reuse-excludes branch from a0e77f9 to d1d5784 Compare May 27, 2025 17:04

Luap99 reviewed May 27, 2025

View reviewed changes

Luap99 approved these changes May 28, 2025

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 28, 2025

mtrmac reviewed May 30, 2025

View reviewed changes

flouthoc force-pushed the reuse-excludes branch from d1d5784 to dc91654 Compare June 3, 2025 15:17

flouthoc force-pushed the reuse-excludes branch from dc91654 to 6b70192 Compare June 3, 2025 17:54

mtrmac reviewed Jun 3, 2025

View reviewed changes

flouthoc force-pushed the reuse-excludes branch from 6b70192 to 51193ef Compare June 3, 2025 19:01

build: reuse parse.ContainerIgnoreFile from buildah

eadded9

podman's logic to parse excludes from `--ignorefile` is not consistent with buildah, use code directly from imagebuilder. Closes: containers#25746 Signed-off-by: flouthoc <flouthoc.git@gmail.com>

flouthoc force-pushed the reuse-excludes branch from 51193ef to eadded9 Compare June 3, 2025 19:03

openshift-ci bot assigned rhatdan Jun 3, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2025

openshift-merge-bot bot merged commit 6a39f37 into containers:main Jun 3, 2025
78 of 79 checks passed

build: reuse parse.ContainerIgnoreFile from buildah #26216

build: reuse parse.ContainerIgnoreFile from buildah #26216

Conversation

flouthoc commented May 27, 2025

Does this PR introduce a user-facing change?

Uh oh!

Luap99 May 27, 2025

Choose a reason for hiding this comment

Uh oh!

flouthoc May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Luap99 May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented May 28, 2025

Uh oh!

flouthoc commented May 30, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac May 30, 2025

Choose a reason for hiding this comment

Uh oh!

flouthoc commented May 30, 2025

Uh oh!

mtrmac commented May 30, 2025

Uh oh!

mtrmac commented May 30, 2025

Uh oh!

flouthoc commented May 31, 2025

Uh oh!

mtrmac commented Jun 2, 2025

Uh oh!

packit-as-a-service bot commented Jun 3, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

flouthoc commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Jun 3, 2025

Uh oh!

flouthoc commented Jun 3, 2025

Uh oh!

rhatdan commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

build: reuse `parse.ContainerIgnoreFile` from buildah #26216

build: reuse `parse.ContainerIgnoreFile` from buildah #26216

flouthoc commented Jun 3, 2025 •

edited

Loading