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.

Sign up for GitHub

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

Jump to bottom

podman 4.4.0 #122296

Closed
wants to merge 5 commits into from
Closed

podman 4.4.0 #122296

wants to merge 5 commits into from

Conversation

xxyzz
Copy link
Contributor

@xxyzz xxyzz commented Feb 4, 2023

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?
  • Build with the latest go
  • Use GNU Make to fix "invalid `override' directive" error
  • Build from git tag to fix "fatal: not a git repository" error
  • Fix build doc error by using just one job with -j1 argument

@xxyzz
podman 4.4.0
edd70c0 
- Build with the latest go
- Use GNU Make to fix "invalid `override' directive" error
- Build from git tag to fix "fatal: not a git repository" error
- Fix build doc error by using just one job with `-j1` argument
@BrewTestBot BrewTestBot added go Go use is a significant feature of the PR or issue rust Rust use is a significant feature of the PR or issue labels Feb 4, 2023
@chenrui333 chenrui333 added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Feb 4, 2023
@p-linnane

This comment was marked as outdated.

Sign in to view
@p-linnane p-linnane closed this Feb 4, 2023
@xxyzz
Copy link
Contributor Author

xxyzz commented Feb 4, 2023

I know there is a pr before but that pr hasn't changed a few days so I want to fix the build error in a new pr. Now at least the build is successful for macOS.

@xxyzz
Copy link
Contributor Author

xxyzz commented Feb 5, 2023

Please reopen and review this pull request, I have pushed two commits to fix the build and test error on Linux. @p-linnane

@p-linnane p-linnane reopened this Feb 5, 2023
@iMichka
Copy link
Member

iMichka commented Feb 5, 2023

Is the -j1 to fix the docs build? We should use the patch from #122122 instead of using this workaround.

@xxyzz
Copy link
Contributor Author

xxyzz commented Feb 5, 2023

Is the -j1 to fix the docs build? We should use the patch from #122122 instead of using this workaround.

That patch won't fix the doc build as the #122122 log shows. podman's Makefile would fail when building with multiple jobs. Please see containers/podman#17367

@iMichka
Copy link
Member

iMichka commented Feb 5, 2023

Can we use your patch?

@xxyzz
Copy link
Contributor Author

xxyzz commented Feb 5, 2023

But that would need to apply two patches and my patch is not merged yet. Besides there is no benefit to build doc with multiple processes as the podman developer said in the first patch: containers/podman@bd8937c

@iMichka
Copy link
Member

iMichka commented Feb 5, 2023

We can ship patches that are not merged yet. We can also ship multiple patches, that's not an issues.

I understand that building docs with multiple processes is probably useless in this case and I can live with that. Upstream could also put the j1 in their build setup instead of us having to carry specific flags around.

We prefer upstream decides / manages their own compiler flags, because if they change their mind, the change will be propagated to us on the next update. Else we will keep useless / wrong flags forever on our side.

If it's too big of a hassle, we can still consider merging as-is, I'm not going to block this pull request on that.

@xxyzz
Copy link
Contributor Author

xxyzz commented Feb 5, 2023

podman's doc doesn't has any flag: https://podman.io/getting-started/installation#build-and-run-dependencies

So they are probably using the default make setting which is executing build tasks one at a time. The -j1 flag actually suppresses the HOMEBREW_MAKE_JOBS flag to restore the default make behavior. I'll add a comment of removing this flag at next release.

@p-linnane p-linnane mentioned this pull request Feb 5, 2023
Closed
iMichka
iMichka approved these changes Feb 5, 2023
View reviewed changes
Copy link
Member

@iMichka iMichka left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@xxyzz xxyzz deleted the podman_4.4 branch February 5, 2023 22:58
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dawidd6 dawidd6 dawidd6 approved these changes

@iMichka iMichka iMichka approved these changes

Assignees
No one assigned
Labels
CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. go Go use is a significant feature of the PR or issue outdated PR was locked due to age rust Rust use is a significant feature of the PR or issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@xxyzz @p-linnane @iMichka @BrewTestBot @dawidd6 @chenrui333