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

Add support for a centos-based distro #202

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jhfrontz
Copy link

@jhfrontz jhfrontz commented Oct 4, 2018

This updates gbuild to be compatible with Centos's package manager (yum). It also adds a few robustness improvements (redirecting cd executions to /dev/null, preventing any shell-prompt-related output from causing confusion).

I've successfully tested/used this with Docker running against an image created with $ bin/make-base-vm --docker --distro centos --suite 7 --arch amd64.

@jhfrontz jhfrontz requested a review from devrandom as a code owner October 4, 2018 02:45
@jhfrontz
Copy link
Author

@achow101 can you review this?

@@ -143,7 +169,7 @@ EOF" if build_desc["sudo"] and @options[:allow_sudo]
build_desc["remotes"].each do |remote|
dir = sanitize(remote["dir"], remote["dir"])

author_date = `cd inputs/#{dir} && git log --format=@%at -1 | date +"%F %T" -u -f -`.strip
author_date = `cd inputs/#{dir} > /dev/null && git log --format=@%at -1 | date +"%F %T" -u -f -`.strip
Copy link
Owner

Choose a reason for hiding this comment

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

what is the output that this is suppressing?

Copy link
Author

Choose a reason for hiding this comment

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

From what I could figure out, the cd was spitting out information about the new directory -- I think in the case where you/your-distro have your shell configured to update the "window title" to reflect the current working directory. Apparently this isn't an issue on the non-Centos invocations?

Copy link
Owner

Choose a reason for hiding this comment

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

This shouldn't happen in non-interactive shells. Perhaps your personal setup has some interactive shell setup or an alias to cd in the wrong shell startup file?

Copy link
Author

Choose a reason for hiding this comment

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

I tracked it down -- it's because I have CDPATH set: https://unix.stackexchange.com/questions/245453/how-to-not-show-path-after-cd-command-with-cdpath-set (note the workaround).

@jhfrontz
Copy link
Author

Note -- I somehow forgot earlier to include the corresponding changes in make-base-vm in my previous commit. I just updated/squashed.

@achow101
Copy link
Contributor

utACK 25630b7

I don't have a CentOS system and am not particularly familiar with them, but this looks correct. At the very least, the debian/ubuntu parts seem to be exactly the same as before.

@jhfrontz
Copy link
Author

Updated to improve compatibility with newer versions of docker.

@@ -46,6 +46,12 @@ def build_one_configuration(suite, arch, build_desc)
ENV["LXC_SUITE"] = suite
end

if ENV["USE_DOCKER"] and build_desc["distro"].eql? "centos"
ontarget_root_extra = "-w /root"
Copy link
Owner

Choose a reason for hiding this comment

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

what happens if we don't supply this argument?

Copy link
Author

Choose a reason for hiding this comment

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

On newer versions of docker there appears to be some sort of security setting that causes the command (without the -w root) to fail because of bad permissions on /home/centos (which is weird, of course, since the login is root). This showed up on a coworker's Arch system -- but only when using a centos-based container.

Similar reports:

@wtogami
Copy link
Contributor

wtogami commented Aug 27, 2019

needs rebase

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

Successfully merging this pull request may close these issues.

4 participants