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

Support layering rpms with files in /opt #1795

Closed
wants to merge 2 commits into from

Conversation

alexlarsson
Copy link
Collaborator

This adds support for layering rpms with files in /opt. The way we
do this is that when importing the rpms we rewrite any files in /opt
into /usr/lib/opt, and then we add back a symlink from the toplevels
of /opt into /usr/lib/opt via the per-package tmpfiles.d.

Also, in order for this to work with the %post script we bind /opt
to usr/lib/opt during the script execution.

This is enough to make layering chrome work.

This fixes #233

@cgwalters
Copy link
Member

This is enough to make layering chrome work.

Hah, cool. So...I'm generally OK with this but it won't help with e.g. Puppet which writes mutable data into /opt too. I think this path is going to lead us down to carrying package-specific "rewrite rules" eventually...

But as for getting Chrome going , sure.

@alexlarsson
Copy link
Collaborator Author

All the tests fail with

error: Server does not allow request for unadvertised object 7ecb2f5ddc93ae6f819b95ef7940b1d4dd66eb4d
Fetched in submodule path 'libdnf', but it did not contain 7ecb2f5ddc93ae6f819b95ef7940b1d4dd66eb4d. Direct fetching of that commit failed.

Any idea what that is about?

@alexlarsson
Copy link
Collaborator Author

So, i had a similar issue locally. The problem is that the url of the libdnf git repo changed and for some reason old checkout just refuse to work as they refer to the old git commit which does not exist in the new upstream. The only way i could make this work was to blow away the old rpm-ostree checkout.

@cgwalters
Copy link
Member

The only way i could make this work was to blow away the old rpm-ostree checkout.

Yeah it took me a while to figure out, you also need to edit the URL in .git/modules/libdnf/config.

@alexlarsson
Copy link
Collaborator Author

@cgwalters How do we fix it for the CI tho?

@jlebon
Copy link
Member

jlebon commented Mar 22, 2019

This PR is modifying the libdnf submodule. Should be good if you just drop that bit from the patch.

@alexlarsson
Copy link
Collaborator Author

Ah, sorry about that, it was caused but the git submodule snafu.

@cgwalters
Copy link
Member

From layering-basic-1:

+ assert_not_reached 'Was able to install a package in /opt'
+ fatal 'Was able to install a package in /opt'
+ echo Was able to install a package in /opt
Was able to install a package in /opt

The cosa one...hmm, may be coreos/coreos-assembler#427

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

The approach looks sane to me! Though as @cgwalters mentioned, this isn't a generic fix for #233, so maybe let's drop the "Fixes #233" commit msg footer?

src/libpriv/rpmostree-importer.c Outdated Show resolved Hide resolved
src/libpriv/rpmostree-scripts.c Outdated Show resolved Hide resolved
tests/vmcheck/test-layering-basic-1.sh Outdated Show resolved Hide resolved
tests/vmcheck/test-layering-basic-1.sh Outdated Show resolved Hide resolved
src/libpriv/rpmostree-util.c Outdated Show resolved Hide resolved
@jlebon jlebon mentioned this pull request Mar 25, 2019
@alexlarsson
Copy link
Collaborator Author

Hmm that fixup is wrong some of it is against the test, not the base commit. Will force push a fix.

@@ -388,6 +389,9 @@ run_script_in_bwrap_container (int rootfs_fd,
/* Scripts can see a /var with compat links like alternatives */
rpmostree_bwrap_var_tmp_tmpfs (bwrap);

if (glnx_fstatat (rootfs_fd, "usr/lib/opt", &stbuf, AT_SYMLINK_NOFOLLOW, NULL) && S_ISDIR(stbuf.st_mode))
rpmostree_bwrap_append_bwrap_argv (bwrap, "--symlink", "usr/lib/opt", "/opt", NULL);
Copy link
Member

Choose a reason for hiding this comment

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

If this works for Chrome, then that's fine! Though I can imagine we'll have to switch to symlinking each /opt subdir here too if scriptlets want to write to other dirs in /opt. Anyway, we can do that later!

This adds support for layering rpms with files in /opt. The way we
do this is that when importing the rpms we rewrite any files in /opt
into /usr/lib/opt, and then we add back a symlink from the toplevels
of /opt into /usr/lib/opt via the per-package tmpfiles.d.

Also, in order for this to work with the %post script we bind /opt
to usr/lib/opt during the script execution.

This fixes coreos#233 at least for Google Chrome.
@jlebon
Copy link
Member

jlebon commented Mar 26, 2019

Awesome, thanks! I just pushed an updated commit message to be more precise that this doesn't fix all of #233.

@rh-atomic-bot r+ 14089ff

@rh-atomic-bot
Copy link

⌛ Testing commit 14089ff with merge d5b9077...

rh-atomic-bot pushed a commit that referenced this pull request Mar 26, 2019
Closes: #1795
Approved by: jlebon
@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing d5b9077 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Determine support path for RPMs that install into /opt, /usr/local etc
4 participants