/snapcraft

New command cleanbuild (using lxd) #328

Merged
merged 1 commit into from Feb 23, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@sergiusens @elopio @chipaca
@sergiusens
Collaborator

sergiusens commented Feb 19, 2016

This new command's intention is to make building with a clean
slate and easy chore.

LP: #1480144

Signed-off-by: Sergio Schvezov sergio.schvezov@ubuntu.com
@sergiusens
Collaborator

sergiusens commented Feb 19, 2016

Hello there, this is cleanbuild, it used to use pylxd but not anymore as the API changed, does not work with python3 with that change and does not have tests anymore.

I skipped adding an integration test as it seemed way to invasive.
snapcraft/commands/cleanbuild.py
+"""
+snapcraft cleanbuild
+
+Uses a container using lxd to create the resulting snap.
@elopio

elopio Feb 19, 2016

Member

Instead of "Uses a container using lxd", just "Uses a lxd container".

@elopio

elopio Feb 19, 2016

Member

Maybe also instead of "to create the resulting snap", just "to build the snap".

@sergiusens

sergiusens Feb 19, 2016

Collaborator

done

snapcraft/commands/cleanbuild.py
+local sources is not using any dependencies local to the developer system.
+
+The cleanbuild command requires a properly setup lxd environment that
+can connect to external networks.
@elopio

elopio Feb 19, 2016

Member

Maybe a link to how to set up a lxd environment?

@sergiusens

sergiusens Feb 19, 2016

Collaborator

done

snapcraft/commands/cleanbuild.py
+
+from snapcraft import repo
+from snapcraft.lxd import Cleanbuilder
+from snapcraft.common import format_snap_name
@elopio

elopio Feb 19, 2016

Member

hum, I dislike the import of symbols instead of modules.
But if you prefer it this way, as long as we are consistent I can live with it. Should we start using this style?

@sergiusens

sergiusens Feb 19, 2016

Collaborator

We already have this all over; it really depends on the length of what you import.

snapcraft/commands/cleanbuild.py
+
+def _create_tar_filter(tar_filename):
+ def _tar_filter(tarinfo):
+ fn = tarinfo.name
@elopio

elopio Feb 19, 2016

Member

why not file_name instead of fn? oh no, you are a go programmer again! :D

@sergiusens

sergiusens Feb 19, 2016

Collaborator

hah, it's because the resulting lines below would have the horrible python semantics due to line length restrictions 😉

snapcraft/commands/cleanbuild.py
+from snapcraft.yaml import load_config
+
+logger = logging.getLogger(__name__)
+_DEFAULT_IMAGE_SERVER = 'https://images.linuxcontainers.org:8443'
@elopio

elopio Feb 19, 2016

Member

This constant is not used here.

snapcraft/lxd.py
+ self._container_name])
+ yield
+ finally:
+ check_call(['lxc', 'stop', self._container_name])
@elopio

elopio Feb 19, 2016

Member

No delete after the build is done?

@sergiusens

sergiusens Feb 19, 2016

Collaborator

It's ephemeral so no need.

snapcraft/lxd.py
+ def _setup_project(self):
+ logger.info('Setting up container with project assets')
+ dst = os.path.join('/root', os.path.basename(self._project))
+ self._push_file(self._project, dst)
@elopio

elopio Feb 19, 2016

Member

I thought it would be nice (and easier) to mount the directory in the container. Then you could inspect the part dirs after the container was killed, and no need to copy things around.

@sergiusens

sergiusens Feb 19, 2016

Collaborator

Mounting requires root though. I prefer this non requiring root scenario as an initial implementation.

snapcraft/lxd.py
+ self._setup_project()
+ self._wait_for_network()
+ self._container_run(['apt-get', 'update'])
+ self._container_run(['apt-get', 'install', 'snapcraft', '-y'])
@elopio

elopio Feb 19, 2016

Member

Oh, I was kind of hoping that we could use the same snapcraft from the host. If we get it from the archive then it's not really useful for testing snapcraft itself.

@sergiusens

sergiusens Feb 19, 2016

Collaborator

The problem with that is that you won't be able to drive builds from a different snapcraft version on a different series.

snapcraft/tests/test_commands_cleanbuild.py
+ self.state_file = os.path.join(common.get_partsdir(), 'part1', 'state')
+
+ @mock.patch('snapcraft.lxd.check_call')
+ def test_cleanbuild(self, mock_call):
@elopio

elopio Feb 19, 2016

Member

You should fake here that lxd is installed, I think.

@sergiusens

sergiusens Feb 19, 2016

Collaborator

I don't understand what this means

@chipaca

chipaca Feb 19, 2016

Member

I think the tests will fail if lxd isn't installed?

snapcraft/common.py
@@ -91,7 +91,7 @@ def get_arch():
return _DEB_TRANSLATIONS[platform.machine()]['arch']
except KeyError:
raise EnvironmentError(
- '{} is not supported, please log a bug at'
+ '{} is not supported, please log a bug at '
@elopio

elopio Feb 19, 2016

Member

You didn't forget, great! :)

@elopio
Member

elopio commented Feb 19, 2016

I like this very much. I left many comments just because I'm cranky and want to start my vacations :)
I would like that invasive integration test. Maybe we can just skip it if lxd is not installed.
snapcraft/commands/cleanbuild.py
+ fn = tarinfo.name
+ if fn.startswith('./parts/') and not fn.startswith('./parts/plugins'):
+ return None
+ elif any(fn == d for d in ('./stage', './snap', tar_filename)):
@chipaca

chipaca Feb 19, 2016

Member

I find fn in ('./stage', './snap', tar_filename) more readable than this

@sergiusens

sergiusens Feb 19, 2016

Collaborator

done

snapcraft/commands/cleanbuild.py
+
+
+def main(argv=None):
+ argv = argv if argv else []
@chipaca

chipaca Feb 19, 2016

Member

argv = [] if argv is None else argv?

@sergiusens

sergiusens Feb 19, 2016

Collaborator

done

snapcraft/common.py
@@ -100,10 +100,16 @@ def get_arch_triplet():
return _DEB_TRANSLATIONS[platform.machine()]['triplet']
except KeyError:
raise EnvironmentError(
- '{} is not supported, please log a bug at'
+ '{} is not supported, please log a bug at '
'https://bugs.launchpad.net/snapcraft'.format(platform.machine()))
@chipaca

chipaca Feb 19, 2016

Member

ooh :-) can I suggest you change the link to go to e.g. https://bugs.launchpad.net/snapcraft/+filebug?field.title=please+add+support+for+{1} ?

@sergiusens

sergiusens Feb 19, 2016

Collaborator

done

snapcraft/lxd.py
+ retry_count -= 1
+ if retry_count == 0:
+ raise e
+ pass
@chipaca

chipaca Feb 19, 2016

Member

pass is spurious there.

Personally I would've done this without the flag variable, but that's probably because I don't like flag variables :-)

while True:
    sleep(5)
    try: ...
    except: ...
    else: break # done
@sergiusens

sergiusens Feb 19, 2016

Collaborator

Yeah, I just find the try/except/else mechanics confusing

@chipaca

chipaca Feb 19, 2016

Member

Fair point.

@chipaca
Member

chipaca commented Feb 19, 2016

👍
@sergiusens
New command cleanbuild (using lxd) 
This new command's intention is to make building with a clean
slate and easy chore.

LP: #1480144

Signed-off-by: Sergio Schvezov <sergio.schvezov@ubuntu.com>

All checks have passed

3 successful checks
@snappy-m-o
Examples tests — Success 13 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.1%) to 93.895%
Details
3e3927a

sergiusens added a commit that referenced this pull request Feb 23, 2016

@sergiusens
Merge pull request #328 from sergiusens/feature/1480144/cleanbuild-lxd 
New command cleanbuild (using lxd)
2f61162

@sergiusens sergiusens merged commit 2f61162 into snapcore:master Feb 23, 2016
3 checks passed

3 checks passed

Examples tests Success 13 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 93.895%
Details

@sergiusens sergiusens deleted the sergiusens:feature/1480144/cleanbuild-lxd branch Feb 23, 2016

smoser pushed a commit to smoser/snapcraft that referenced this pull request Sep 14, 2016

@hglkrijger
Merge pull request #328 from hglkrijger/master 
update agent to 2.1.5
eb082c4

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

@sergiusens
Merge pull request #328 from sergiusens/feature/1480144/cleanbuild-lxd 
New command cleanbuild (using lxd)
ab6de19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment