Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Task to build core snaps in launchpad #49
Conversation
sergiocazzolato
added some commits
Sep 25, 2017
sergiocazzolato
requested review from
mvo5,
stolowski,
zyga and
niemeyer
Oct 3, 2017
sergiocazzolato
changed the title from
Task to build core snap in launchpad
to
Task to build core snaps in launchpad
Oct 3, 2017
mvo5
reviewed
Oct 4, 2017
Thanks for working on this! This looks very good, some questions/suggestions inline.
| + | ||
| +# we need to store credentials once for cronned builds | ||
| +cachedir = os.path.join(workdir, 'cache') | ||
| +creds = os.path.join(workdir, 'credentials') |
mvo5
Oct 4, 2017
Contributor
How will those credentials be injected here? And from what user (we have a function account for this, right?)
sergiocazzolato
Oct 4, 2017
•
Edited 1 time
-
sergiocazzolato
Oct 4, 2017
Contributor
-
Oct 4, 2017
I planned to add them manually once the branch is merged. I have to use an account with the permissions to trigger builds, and I though to use mine due these credentials can be used just on that machine. Does it make sense? any other idea?
niemeyer
Oct 23, 2017
How would that work? Will we have a machine sitting somewhere that can release core snaps arbitrarily?
sergiocazzolato
Oct 23, 2017
Contributor
@niemeyer this will run in the same machine where we run the spread-cron branches. The access to this machine is restricted of course.
niemeyer
Oct 23, 2017
It's a CI machine getting a non-trivial amount of tip source code and running it on external systems. Unless there are more clear details about how "restricted of course" it is, I'd be super uncomfortable in trusting the security of the core snap to it.
ogra1
Oct 23, 2017
@niemeyer note that lp-build-core.py has originally been one of my shellscripts for the daily cronned build setup that i then ported to python (to make use of launchpad-lib), as such it indeed still looks like shell ;) when i gave it to sergio it was more meant as a suggestion... this should surely get re-worked a lot when integrating it somewhere in public code ...
this script simply triggers a pre-defined snap build in LP, so the "machine sitting somewhere" doing the releases is launchpad itself ... but ... unlike the set-up i use for the daily cronned builds the credentials are in your case not on a private machine (in my case it is people.canonical.com, in a dir not accessible by any outside person) so i dont really see how the credentials setup will work without leaking them to the outside world ...
niemeyer
Oct 24, 2017
For the record, shell scripts do have functions as well, and scoping. Non-trivial scripts ought to make good use of those.
| + mybuilds.append(buildid) | ||
| + print('Arch: {} is building under: {}'.format(buildarch, request)) | ||
| + | ||
| +# check the status each minute til all builds have finished |
| + for build in mybuilds: | ||
| + try: | ||
| + response = ubuntucore.getBuildSummariesForSnapBuildIds(snap_build_ids=[build]) | ||
| + except: |
mvo5
Oct 4, 2017
Contributor
We should log the exception here, something like (untested):
except e:
print('could not get response for {} (was there an LP timeout ?): {}'.format(build, e))
| + failure, buildlog)) | ||
| + built_arches.remove(arch) | ||
| + | ||
| +# save a file with a line containing all the architectures that were built successfully |
| @@ -1,3 +1,2 @@ | ||
| -pattern_extractor="curl -s https://api.travis-ci.org/repos/snapcore/snapd/builds?event_type=push | jq -j 'map(select(.result == 0) | select(.branch == \"master\")) | .[0].number'" |
mvo5
Oct 4, 2017
Contributor
Hm, I'm slightly confused why there is an options file here that is overriden. Shouldn't the build-core-lp branch that we use (and target) be empty?
sergiocazzolato
Oct 4, 2017
Contributor
I created the branch build-core-lp on snapcore/spread-cron to have one branch to compare in the pull request, but that branch is set as manual in the options file which means the branch is not executed by spread-cron. The final options file is the one you have here in the PR.
sergiocazzolato
Oct 4, 2017
Contributor
I should have been created a PR for this manual branch I guess
| +export RESULTS_FILE=built_architectures.txt | ||
| + | ||
| +# build core for all the architectures | ||
| +output=0 |
mvo5
Oct 4, 2017
Contributor
Maybe result or res or exitcode instead of output? output sounds like some textual output to me.
sergiocazzolato
referenced this pull request
Oct 4, 2017
Closed
Adding commits information on snapd-vendor launchpad repo #48
sergiocazzolato
added some commits
Oct 4, 2017
| +#!/bin/sh | ||
| + | ||
| +mkdir -p bin | ||
| +( cd bin && wget https://niemeyer.s3.amazonaws.com/spread-amd64.tar.gz -O spread.tar.gz && tar xzvf spread.tar.gz ) |
ogra1
Oct 5, 2017
•
Edited 1 time
-
ogra1
Oct 5, 2017
-
Oct 5, 2017
how about:
wget https://niemeyer.s3.amazonaws.com/spread-amd64.tar.gz -O - | zcat - | tar x
that saves disk space by directly streaming the tarball to disk (and you dont need to clean up temporary files ;) )
sergiocazzolato
added some commits
Oct 5, 2017
niemeyer
requested changes
Oct 23, 2017
It would be nice to have some minimum structure in that code. Note how there are zero functions, and every variable is a global. This is hard to read, maintain, review, reason about, etc.
|
@niemeyer sure, you mean in the python script? I'll work on that. Thanks for reviewing this. |
zyga
reviewed
Oct 24, 2017
A few quick comments. I'll have more once I know if this is python 2.7 or python 3.x
| @@ -0,0 +1,146 @@ | ||
| +#!/usr/bin/python | ||
| + |
zyga
Oct 24, 2017
If this is python 2.x can you please add
from __future__ import absolute_import, print_function
If this is python3 then please adjust the first line to read #!/usr/bin/python3
ogra1
Oct 24, 2017
•
Edited 1 time
-
ogra1
Oct 24, 2017
-
Oct 24, 2017
originally this is python2 because it runs on people.canonical.com which is a 12.04 machine ...
also, since it has not been mentioned, the original script (and the README with basic instructions) is at:
https://github.com/snapcore/core/tree/master/cron-scripts
| + while len(self.builds): | ||
| + elapsed_build_time = int((datetime.now() - self.start_build_time).total_seconds()) | ||
| + if timeout < elapsed_build_time: | ||
| + print('Timeout reached') |
zyga
Oct 24, 2017
Not that it strictly matters here but this could be some kind of new TimeoutError or even a plain old SystemExit
| + | ||
| + if status == 'FULLYBUILT': | ||
| + self.builds.remove(build) | ||
| + continue |
zyga
Oct 24, 2017
The continue statements in each of the branches here are no-ops since this is the last statement in the loop body.
sergiocazzolato commentedOct 3, 2017
•
Edited 1 time
-
sergiocazzolato
Oct 4, 2017
This job triggers the builds in launchpad for the different
architectures every night, and tracks the relation between the build and
the commit in git to be used to run tests.
Still missing to implement a change in master to add the dependencies needed for this branch.
python packages: launchpadlib and urlparse2