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 new "no-wrapper" property to apps #1420

Merged
merged 8 commits into from Oct 2, 2017

Conversation

5 participants
@mvo5
Contributor

mvo5 commented Jul 26, 2017

A RFC branch to fix https://bugs.launchpad.net/snapcraft/+bug/1706575

This allows to specify that no shell wrapper for apps should be generated. This is useful for e.g. statically linked binaries that don't need this anyway.

LP: 1706575

@zyga

Small future-proof suggestion

@@ -190,6 +190,10 @@ properties:
- type: string
minLength: 1
- type: number
no-wrapper:

This comment has been minimized.

@zyga

zyga Jul 26, 2017

Contributor

Suggestion: make this "wrappers" enum that has possible values "legacy" and "none" as we may be able to provide some kind of useful wrappers later on.

This comment has been minimized.

@sergiusens

sergiusens Jul 26, 2017

Collaborator

Agreed, this should be an array of unique elements like build-attributes in parts.

@sergiusens

Thanks for the improvement; I agree with @zyga on this and we have long shied away from boolean entries to be future proof. it would be nice to see an integration test that creates a snap and verifies no wrapper is used in the end.

@@ -10,6 +10,8 @@ apps:
command: binary1
binary2:
command: subdir/binary2
binary-no-wrapper:
command: subdir/binary3

This comment has been minimized.

@elopio

elopio Jul 28, 2017

Member

This is missing:
wrapper: none

@@ -73,6 +73,10 @@ def test_snap(self):
os.path.join(self.prime_dir, 'bin', 'not-wrapped.wrapper'),
Not(FileExists()))
self.assertThat(
os.path.join(self.prime_dir, 'bin', 'command-binary3.wrapper'),

This comment has been minimized.

@elopio

elopio Jul 28, 2017

Member

The name of the file should be: command-binary-no-wrapper.wrapper.

@mvo5

This comment has been minimized.

Contributor

mvo5 commented Aug 9, 2017

Thanks for the feedback and sorry for my slow reply. I addressed the review feedback now. Please let me know if I can help in any other way.

@mvo5 mvo5 force-pushed the mvo5:no-wrapper branch from 58130c4 to 8ffbcc6 Aug 9, 2017

@zyga

zyga approved these changes Aug 9, 2017

Small comment, looks good to me

@@ -190,6 +190,13 @@ properties:
- type: string
minLength: 1
- type: number
wrapper:

This comment has been minimized.

@zyga

zyga Aug 9, 2017

Contributor

Nice :-)

self._wrap_app(app, apps[app])
wrapper = "legacy"
if "wrapper" in apps[app]:
wrapper = apps[app]["wrapper"]

This comment has been minimized.

@zyga

zyga Aug 9, 2017

Contributor

wrapper = apps[app].get("wrapper", legacy) is shorter

Not sure if snapcraft uses anything that can derive the default from the schema.

@sergiusens

This comment has been minimized.

Collaborator

sergiusens commented Aug 10, 2017

Hi, thanks for the changes, I was thinking of something more like:

apps:
    no-wrapper-app:
        command: app
        app-constraints: [no-wrapper]
    wrapped-app
        command: app

wdyt?

@sergiusens

This comment has been minimized.

Collaborator

sergiusens commented Aug 10, 2017

Now that I think of it, it no-wrapper might even be no-environment as we already have plans to get rid of wrappers and make use of env for apps a lot more.

@mvo5

This comment has been minimized.

Contributor

mvo5 commented Aug 17, 2017

I updated the PR now to follow the suggestion of Gustavo in https://forum.snapcraft.io/t/telling-snapcraft-to-skip-generating-wrapper-scripts/1635/3 - i.e. it is using: adapter: {"", "none"} now.

@mvo5

This comment has been minimized.

Contributor

mvo5 commented Aug 28, 2017

How are things looking? I updated the code based on the feedback that I got from @niemeyer. Please let me know if there is anything else I can do to help this PR along, I need it for snapcore/snapd#3625 so it woudl be great if it could make it to the next snapcraft release.

@kyrofa

This feels a little odd to have a valid enum value as "". It's an optional field, right? Why not just leave it off in such a case? I can't imagine people will actually use adapter: '' anywhere.

@kyrofa

This comment has been minimized.

Member

kyrofa commented Sep 19, 2017

Ping on this one.

review no longer valid

@mvo5

This comment has been minimized.

Contributor

mvo5 commented Sep 21, 2017

Thanks, updated and removed the "" from the enum.

Fixes requested have been completed.

@kyrofa

kyrofa approved these changes Sep 28, 2017

With the changes, this looks great, thanks @mvo5!

sergiusens added some commits Oct 1, 2017

Update meta.py
single quotes

@sergiusens sergiusens merged commit 2a14e16 into snapcore:master Oct 2, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sergiusens sergiusens added this to the 2.35 milestone Oct 2, 2017

@sergiusens sergiusens added this to Integration in 17.10 Oct 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment