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: rename build artifact for windows and darwin #9918

Closed
wants to merge 1 commit into from

Conversation

vrothberg
Copy link
Member

@vrothberg vrothberg commented Apr 1, 2021

Rename podman-remote to podman.exe on Windows and to podman on Mac
OS. The -remote suffix isn't needed since Podman does not run
natively on these systems.

Signed-off-by: Valentin Rothberg [email protected]

@baude
Copy link
Member

baude commented Apr 1, 2021

lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

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

@vrothberg
Copy link
Member Author

Requested in crc-org/crc#2171.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2021
@vrothberg
Copy link
Member Author

Needs some more massaging

@mheon
Copy link
Member

mheon commented Apr 1, 2021

@cevich PTAL - will this break the Cirrus jobs that build binary artifacts?

@cevich
Copy link
Member

cevich commented Apr 1, 2021

I don't think so, but thanks for checking. The problem I intended to fix in #9381 is more related to the order of targets. Though @vrothberg be warned...this area of the Makefile is very delicate 😕 My plan is to get back onto that PR, after my get_ci_vm wounds scab over 😁

@TomSweeneyRedHat
Copy link
Member

@vrothberg unhappy red tests

Rename `podman-remote` to `podman.exe` on Windows and to `podman` on Mac
OS.  The `-remote` suffix isn't needed since Podman does not run
natively on these systems.

Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
Copy link
Member Author

@rhatdan @gbraad WDYT?

@gbraad
Copy link
Member

gbraad commented Apr 2, 2021

We will deliver integration of Podman with CRC, but the current binary names aren't consistent:

For example, the V2.2.1 releases are:

This would mean that the binary on Windows is named podman. This would indicate the binary does more than just being a remote client. While for use on Linux that would make sense. I understand it is not an aesthetically pleasing solution, as we want to increase the notion of podman... but in short, we will also be lying to the user. On Linux this would make sense, as we want ot prevent a clash with a possible installed version of the actual podman command.

I would also like to see consistent naming in the release artifacts, as the current artifacts include -release for Windows and macOS

@vrothberg
Copy link
Member Author

Thanks, @gbraad!

I'll wait a bit for consensus until pushing the PR forward.

@rhatdan
Copy link
Member

rhatdan commented Apr 2, 2021

Podman-remote on a MAC should be called podman.
Podm,remote.exe on a Windows box should be called podman.exe

Users think in terms of Podman, they don't really care if it works locally or remote. Only on linux should we call podman-remote podman-remote, since native podman is there and almost no-one will ever use podman-remote on a linux box, unless you want a smaller binary and only want to use remote connections.

I think we should just make make create bin/windows/podman.exe and bin/darwin/podman.

@cevich
Copy link
Member

cevich commented Apr 2, 2021

bin/windows/podman.exe and bin/darwin/podman.

I like that idea, it might allow simplifying the Makefile (which I plan to work on soon-ish in #9381). i.e. We already have targets for bin/podman and bin/podman-remote. Speaking of which, should we also do: bin/linux/podman and bin/linux/podman-remote for consistency sake?

@rhatdan
Copy link
Member

rhatdan commented Apr 2, 2021

Sure, although it would drive me crazy, since I am used to typing ./bin/podman

@vrothberg
Copy link
Member Author

vrothberg commented Apr 2, 2021

Sure, although it would drive me crazy, since I am used to typing ./bin/podman

I think that make podman{-remote} should continue placing the artifacts as before. Otherwise, we'd likely break the builds of most (all?) distros.

@cevich
Copy link
Member

cevich commented Apr 2, 2021

we'd likely break the builds of most (all?) distros

It's easy enough to modify the CI scripts and tests, but you do raise a good point. There may be third-party automation (like rpm or deb builds) that would need to be modified. So yeah, with that in mind, we probably should leave it as-is w/ everything under bin.

Alternate idea for Mac (maybe Windows?): Call the binary podman-remote, but make a symlink to it called podman. That could allow for some clarity of function/purpose while still permitting a reduction in typing 😁

@vrothberg
Copy link
Member Author

I am less worried about Windows and Mac builds, since we provide these upstream. But the Linux binaries have many downstream users.

@mheon
Copy link
Member

mheon commented Apr 2, 2021 via email

@cevich
Copy link
Member

cevich commented Apr 6, 2021

I’d be OK with placing non-Linux binaries in subdirectories at build time
(will make things much simpler IMO).

I've now done this in #9381 and got it working. It turned out to not be so simple, but it does prevent any mixups of the binaries from one GOOS accidentally being run on another. Anyway, I think it's okay to close this PR and I'll work on incorporating it into mine.

@rhatdan rhatdan closed this Apr 7, 2021
@vrothberg vrothberg deleted the remote.exe branch April 7, 2021 07:19
@vrothberg
Copy link
Member Author

Thank you, @cevich !

cevich added a commit to cevich/podman that referenced this pull request Apr 12, 2021
* Incorporate changes from abandoned containers#9918: Use dedicated `bin`
  sub-directories for `windows` and `darwin` when building
  `podman-remote`.  The linux flavor remains under `bin` as before.

* Fix MacOS Documentation-generation for release-packaging.
  The `install-podman-remote-%-docs` target requires local execution
  of `podman-remote`, but it was assuming GOOS=linux.  Fix this
  by dynamically discovering the local OS/architecture type while
  still permitting cross-building of MacOS binaries under Linux.

* Unify temporary directory/file behavior to use a common template.
  In case of left-over temporary items left in the repository,
  update the `clean` target accordingly to remove them.

* Fix broken podman-remote-static and MacOS release archive targets
  mismatching the `podman-remote-%` target.  Disambiguate this target
  for all platforms by spelling each out in full, instead of using
  a wild-card recipe.

* Fix Windows-installer target to properly recognize existing
  output files and not constantly rebuild every time.

* Include the podman version number in the Windows-installer target
  in case a user downloads multiple releases.

* Include a subdirectory containing the podman version number for
  both `tar.gz` and `zip` targets.  This prevents users clobbering
  existing directories when un-archiving from releases.

Signed-off-by: Chris Evich <[email protected]>
jmguzik pushed a commit to jmguzik/podman that referenced this pull request Apr 26, 2021
* Incorporate changes from abandoned containers#9918: Use dedicated `bin`
  sub-directories for `windows` and `darwin` when building
  `podman-remote`.  The linux flavor remains under `bin` as before.

* Fix MacOS Documentation-generation for release-packaging.
  The `install-podman-remote-%-docs` target requires local execution
  of `podman-remote`, but it was assuming GOOS=linux.  Fix this
  by dynamically discovering the local OS/architecture type while
  still permitting cross-building of MacOS binaries under Linux.

* Unify temporary directory/file behavior to use a common template.
  In case of left-over temporary items left in the repository,
  update the `clean` target accordingly to remove them.

* Fix broken podman-remote-static and MacOS release archive targets
  mismatching the `podman-remote-%` target.  Disambiguate this target
  for all platforms by spelling each out in full, instead of using
  a wild-card recipe.

* Fix Windows-installer target to properly recognize existing
  output files and not constantly rebuild every time.

* Include the podman version number in the Windows-installer target
  in case a user downloads multiple releases.

* Include a subdirectory containing the podman version number for
  both `tar.gz` and `zip` targets.  This prevents users clobbering
  existing directories when un-archiving from releases.

Signed-off-by: Chris Evich <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
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. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants