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

charm-build: Cleanup directory handling and fix exploding nested build artifacts #478

Merged
merged 9 commits into from Feb 4, 2019

Conversation

Projects
None yet
3 participants
@johnsca
Copy link
Member

johnsca commented Jan 29, 2019

This makes the handling of directories during charm build more consistent, improves the environment variable naming by using consistent prefixes, moves the build cache to live under ~/.cache/charm by default, and fixes #475 by blocking the build output and cache directories from being nested under the source directory which led to recursive copying of build artifacts.

One change that might require some immediate user change is that the default build directory for charms that don't specify a series in their metadata.yaml and are built without an explicit --series flag on the CLI changes from trusty to builds. However, this can be managed in several ways, with the recommended being adding the series field to metadata.yaml and specifying CHARM_BUILD_DIR in the environment (this replaces JUJU_REPOSITORY and the inconsistent and implicit subdirectory of either builds or the series name).

@johnsca

This comment has been minimized.

Copy link
Member Author

johnsca commented Jan 30, 2019

This is available to test in branch edge/bug-475 in the snap store for amd64 arch.

snap refresh charm --channel edge/bug-475
Show resolved Hide resolved charmtools/build/builder.py Outdated

johnsca added some commits Jan 28, 2019

Clean up directory management
Make directory options and environment variables consistent, use
prefixes on environment variables, and move build cache to
~/.cache/charm.

Also, make it a fatal error to have the build output or cache
directories nested under the source directory (fixes #475).
Fix order-dependent test failure
Also fix output for non-local layers, and make LayerFetcher the base
instead of InterfaceFetcher, as it makes more sense.

@johnsca johnsca force-pushed the bug/475/matryoshka-build-dirs branch from 48095c9 to f31d680 Feb 1, 2019

@knkski

knkski approved these changes Feb 1, 2019

Copy link

knkski left a comment

LGTM. Built a couple of the kubeflow charms successfully with this PR.

if cls.OLD_ENVIRON in os.environ and cls.ENVIRON not in os.environ:
log.warning('DEPRECATED: {} environment variable; '
'please use {} instead'.format(cls.OLD_ENVIRON,
cls.ENVIRON))

This comment has been minimized.

@Cynerva

Cynerva Feb 1, 2019

Do we want a deprecation warning for JUJU_REPOSITORY too?

series = self.series or 'builds'
log.warn('Build dir not specified via command-line or '
'environment; defaulting to /tmp/charm-builds')
self.build_dir = path('/tmp/charm-builds')

This comment has been minimized.

@Cynerva

Cynerva Feb 1, 2019

I'm vaguely concerned that /tmp/charm-builds as a default could lead to conflicts in multi-user environments. But it's certainly not a big deal - charm build makes it pretty obvious that you can configure the build dir if needed.

This comment has been minimized.

@johnsca

johnsca Feb 2, 2019

Author Member

Yeah, I considered doing $HOME/charm-builds or something, but I didn't want to accidentally clobber anything with this change. Also, it's better if the user is explicit, anyway, hence the warning.

@Cynerva

Cynerva approved these changes Feb 1, 2019

Copy link

Cynerva left a comment

I had a couple minor comments but this LGTM as it is. The new default behavior makes more sense and the new environment variables have much better names. 👍

@johnsca johnsca merged commit fd999e3 into master Feb 4, 2019

2 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

johnsca added a commit that referenced this pull request Feb 4, 2019

Restore historical behavior for --output-dir
Previous to #478, the behavior of `--output-dir` matched that of
`$JUJU_REPOSITORY` in that it specified a directory which would have a
`builds` or `{series}` directory created under it, which would then have
the final charm output dir created under that.  This got changed and
broke the charm builds for OpenStack, so this restores the previous
behavior of `--output-dir` while retaining the new semantics for
`--build-dir`.

johnsca added a commit that referenced this pull request Feb 5, 2019

Restore historical behavior for --output-dir (#480)
* Fix install instructions in README

* Make JUJU_REPOSITORY only a fallback for layers

Only look for layers / interfaces in JUJU_REPOSITORY if other, more
explicit env vars are not set.

* Restore historical behavior for --output-dir

Previous to #478, the behavior of `--output-dir` matched that of
`$JUJU_REPOSITORY` in that it specified a directory which would have a
`builds` or `{series}` directory created under it, which would then have
the final charm output dir created under that.  This got changed and
broke the charm builds for OpenStack, so this restores the previous
behavior of `--output-dir` while retaining the new semantics for
`--build-dir`.

* Fix build / cache dir checks when parents are missing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment