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.

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

Makefile: do not hardcode GOOS in podman-remote-static target #22388

Merged
merged 1 commit into from
May 13, 2024
Merged

Makefile: do not hardcode GOOS in podman-remote-static target #22388

merged 1 commit into from
May 13, 2024

Conversation

xduugu
Copy link
Contributor

@xduugu xduugu commented Apr 15, 2024

Follow-up of #22060 (comment)

Does this PR introduce a user-facing change?

None

Signed-off-by: Cedric Staniewski <cedric@gmx.ca>
@rhatdan
Copy link
Member

rhatdan commented Apr 16, 2024

/approve
LGTM
@containers/podman-maintainers PTAL
Thanks @xduugu

Copy link
Contributor

openshift-ci bot commented Apr 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, xduugu

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2024
@mheon
Copy link
Member

mheon commented Apr 16, 2024

@cevich PTAL

@baude
Copy link
Member

baude commented Apr 16, 2024

please use an actual commit message on why we need to do this.

@cevich
Copy link
Member

cevich commented Apr 16, 2024

Yes, please write up a complete commit message and PR description. Though I'm glad to see CI passing with this, I don't recall off-hand if any CI builds exercise any of the affected targets. I can double-check once the commit message is ready.

@edsantiago
Copy link
Member

Careful - this was added in #14444 to fix #14201, which is a Windows thing, which means it's probably not tested or exercised in CI.

@cevich
Copy link
Member

cevich commented Apr 17, 2024

Thanks for the archaeology and confirming Ed. Though IIRC, Windows builds are in a weird CI situation, where we test the installer using a cross-compiled binary but the official release builds come from the winmake powershell script. The later I don't believe runs a build anywhere in CI.

@xduugu
Copy link
Contributor Author

xduugu commented Apr 17, 2024

@baude What do you have in mind? It actually never was required to restrict the OS in podman-remote-static. I just did not remove it when I added the target in #22060, so should I just write this into the commit message?

@edsantiago The podman-remote-static target in #14444 is not the same as the one that I added in #22060. The CI most likely uses the release-artifacts target which again calls make podman-remote-static-linux_amd64 and make podman-remote-static-linux_arm64 to generate the binaries.

The history is:

@cevich
Copy link
Member

cevich commented Apr 19, 2024

The CI most likely uses the

From various points CI runs:

  • make podman-release
  • make podman-remote-release-windows_amd64.zip (Linux cross-compile)
  • make podman-release-${arch}.tar.gz GOARCH=$arch for a large set of $arch values
  • make podman-remote (OSX)
  • make podman-remote-release-darwin_amd64.zip and make podman-remote-release-darwin_arm64.zip

I did not track through the Makefile to see if any of those land down $(SRCBINDIR)/podman-remote-static-linux_% (with some value for %)

@TomSweeneyRedHat
Copy link
Member

@cevich @edsantiago @baude thoughts on what to do with this one?

@cevich
Copy link
Member

cevich commented May 7, 2024

Oh sorry I completely forgot about this. IIRC this gives us the option to produce static podman-remote binaries, have I got that right? I think this change is syntactically correct, if somebody else could confirm we're actually touching the $(SRCBINDIR)/podman-remote-static-linux_% target in CI somewhere. If so then this LGTM.

@rhatdan
Copy link
Member

rhatdan commented May 13, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit aff18ba into containers:main May 13, 2024
94 checks passed
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 12, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Aug 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants