Skip to content

build: Use rpm-ostree compose --print-only to expand ${basearch} #192

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.

Sign up for GitHub

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

Merged
jlebon merged 1 commit into from
Nov 5, 2018
Merged

build: Use rpm-ostree compose --print-only to expand ${basearch} #192

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions src/cmdlib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,14 @@ prepare_build() {

echo "Using manifest: ${manifest}"

manifest_tmp_json=${workdir}/tmp/manifest.json
rpm-ostree compose tree --repo=repo --print-only ${manifest} > ${manifest_tmp_json}
Copy link
Copy Markdown
Member

@dustymabe dustymabe Nov 5, 2018

Choose a reason for hiding this comment

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

@jlebon does this extra rpm-ostree compose invocation have implications on the supermin PR?

Copy link
Copy Markdown
Member

@jlebon jlebon Nov 5, 2018

Choose a reason for hiding this comment

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

That should be fine; --print-only should work without any privileges at all.

Copy link
Copy Markdown
Member

@dustymabe dustymabe Nov 5, 2018

Choose a reason for hiding this comment

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

what i was referring to is the repo we operate against. Will it perform any write operations to the repo. Do we need to spin up a VM to do this in the supermin case for consistency?

Copy link
Copy Markdown
Member

@jlebon jlebon Nov 5, 2018

Choose a reason for hiding this comment

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

Will it perform any write operations to the repo.

If it does, that's something we would fix in rpm-ostree. :)


# Abuse the rojig/name as the name of the VM images
export name=$(manifest_get '["rojig"]["name"]')
export name=$(jq -r '.rojig.name' < ${manifest_tmp_json})
# TODO - allow this to be unset
export ref=$(manifest_get '["ref"]')
export ref=$(jq -r '.ref' < ${manifest_tmp_json})
rm -f ${manifest_tmp_json}
Copy link
Copy Markdown
Contributor

@lucab lucab Nov 5, 2018

Choose a reason for hiding this comment

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

Any reason why we don't quote variables (here and everywhere else too)?

Copy link
Copy Markdown
Member Author

@cgwalters cgwalters Nov 5, 2018

Choose a reason for hiding this comment

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

Yeah, consistency is not strong on this. Too bad there's not a "strict mode" for this. But probably our best bet really is to rewrite this in not-bash...

Copy link
Copy Markdown
Member Author

@cgwalters cgwalters Nov 5, 2018

Choose a reason for hiding this comment

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

I definitely do think about quoting whenever a variable "could reasonably" have spaces; definitely when handling externally provided filename arguments for example.

But in practice we don't support spaces for these variables like workdir, ref, name etc. So my inclination is to go with this as is - that OK with you?

Copy link
Copy Markdown
Contributor

@lucab lucab Nov 5, 2018

Choose a reason for hiding this comment

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

Sure, I was mostly asking for my own knowledge in case I was missing some other details, not a blocker. It makes sense to bucket this under #161.

On other coreos projects we use https://github.com/koalaman/shellcheck, which helps catching simple things.

Copy link
Copy Markdown
Member Author

@cgwalters cgwalters Nov 5, 2018

Choose a reason for hiding this comment

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

Ooh, wow. That is a useful project. I just ran it against cmdlib.sh and indeed...lots to fix.


# This dir is no longer used
rm builds/work -rf
Expand All @@ -90,11 +94,6 @@ prepare_build() {
export TMPDIR=$(pwd)/tmp
}

# We'll rewrite this in a real language I promise
manifest_get() {
python3 -c 'import sys,yaml; print(yaml.safe_load(open(sys.argv[1]))'"$1"')' "${manifest}"
}

runcompose() {
local treecompose_args=""
if ! grep -q '^# disable-unified-core' "${manifest}"; then
Expand Down