Skip to content

Conversation

@llchan
Copy link
Contributor

@llchan llchan commented Jun 7, 2019

We install our podman in a non-standard prefix, e.g. PREFIX=/path/to/podman-1.3.1, and during the installation process we stage the files in a DESTDIR=/path/to/sandbox. This changeset makes the DESTDIR apply even if PREFIX is specified. It also makes the constants in the binary prefix-aware, so it's able to find its own conf files rather than the hardcoded /usr/local path. We could also make it search by relative path to the binary, but that's a little more involved.

One thing to note is that some of the constants were converted to vars so that they can be overridden at build time. I don't think this should be an issue, but worth mentioning.

Work in progress, just wanted to push this up for comments/discussion.

@openshift-ci-robot
Copy link
Collaborator

Hi @llchan. Thanks for your PR.

I'm waiting for a containers or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 7, 2019
@mheon
Copy link
Member

mheon commented Jun 7, 2019

I don't know if I like version.go for the prefixes (runtime.go would keep everything in one place), but overall I have no problems with this - though I suspect there are a lot of lingering hardcoded /etc/ and /usr/share paths strewn throughout the code.

@rh-atomic-bot
Copy link
Collaborator

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

@llchan
Copy link
Contributor Author

llchan commented Jun 7, 2019

Yeah, I actually had it originally in runtime.go, but stuck it in version.go to sit alongside the other build-time constants. Can easily move it if that's preferred.

@llchan
Copy link
Contributor Author

llchan commented Jun 7, 2019

Also FYI, my heuristic for which paths to touch was "can the Makefile install something to this path", so stuff like runc/conmon paths were left alone, but the podman config files were made relative to the install prefix. I wasn't quite sure what to do with the cataonit stuff so I left it alone. Once we make the catatonit install less hacky and have it respect the Makefile PREFIX, I think we should also make the default init path relative.

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #3282) made this pull request unmergeable. Please resolve the merge conflicts.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 7, 2019
@rhatdan
Copy link
Member

rhatdan commented Jun 8, 2019

@llchan Needs a rebase.

@rhatdan
Copy link
Member

rhatdan commented Jun 8, 2019

LGTM

@llchan llchan force-pushed the improve-prefix-handling branch from cb5218c to 165d186 Compare June 9, 2019 22:54
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2019
@llchan llchan force-pushed the improve-prefix-handling branch from 165d186 to 8675eab Compare June 9, 2019 22:59
@llchan
Copy link
Contributor Author

llchan commented Jun 9, 2019

Rebased and re-pushed, and added missing sign-off to the second commit. Can squash if preferred.

Copy link
Member

Choose a reason for hiding this comment

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

Go Lint does not like the string here.
installPrefix = "/usr/local"
is preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, and updated my vim setup to lint in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Go Lint does not like the string here.
etcDir = "/etc"
is preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@mheon
Copy link
Member

mheon commented Jun 10, 2019

/ok-to-test
/approve
LGTM

@openshift-ci-robot openshift-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 10, 2019
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: llchan, mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2019
@rhatdan
Copy link
Member

rhatdan commented Jun 10, 2019

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 10, 2019
@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #3328) made this pull request unmergeable. Please resolve the merge conflicts.

@llchan llchan force-pushed the improve-prefix-handling branch from 8d9a6b0 to c581a83 Compare June 14, 2019 22:40
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2019
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2019
llchan added 3 commits June 14, 2019 17:42
- PREFIX is now passed saved in the binary at build-time so that default
  paths match installation paths.
- ETCDIR is also overridable in a similar way.
- DESTDIR is now applied on top of PREFIX for install/uninstall steps.
  Previously, a DESTDIR=/foo PREFIX=/bar make would install into /bar,
  rather than /foo/bar.

Signed-off-by: Lawrence Chan <[email protected]>
@llchan llchan force-pushed the improve-prefix-handling branch from c581a83 to 7baa6b6 Compare June 14, 2019 22:42
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2019
@llchan
Copy link
Contributor Author

llchan commented Jun 14, 2019

Rebased

@rhatdan
Copy link
Member

rhatdan commented Jun 15, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2019
@rhatdan
Copy link
Member

rhatdan commented Jun 15, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2019
@openshift-merge-robot openshift-merge-robot merged commit b82c4e9 into containers:master Jun 15, 2019
@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 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 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. 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. ok-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants