Skip to content
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 additional content #15

Closed

Conversation

brlin-tw
Copy link
Contributor

This patch adds additional content to the template for convenience.

Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
This patch introduces the /snap/patches directory for packagers to place
patches used in the packaging process.  A `patches` part is included to
install these files under $SNAPCRAFT_STAGE/packages to be used by other
parts.

A Sed script is also included to patch the desktop entries' name and
icon.

Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
For adding screenshots.

Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
Copy link
Member

@lucyllewy lucyllewy left a comment

Choose a reason for hiding this comment

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

I've left a few comments on things I'd like to be changed...

@@ -15,12 +15,12 @@ distributions.</p>

## Install

sudo snap install my-snap-name
sudo snap install --channel=edge --devmode my-snap-name
Copy link
Member

Choose a reason for hiding this comment

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

I think we want this to remain as-is.

Copy link
Member

Choose a reason for hiding this comment

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

I agee. Please revert this change @Lin-Buo-Ren


([Don't have snapd installed?](https://snapcraft.io/docs/core/install))

<!-- Uncomment and modify this when you have a screenshot
![my-snap-name](screenshot.png?raw=true "my-snap-name")
![my-snap-name](snap/screenshots/screenshot.png?raw=true "my-snap-name")
Copy link
Member

Choose a reason for hiding this comment

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

Files in /snap/ will be copied into the package. They cannot be filtered via stage: or prime: keywords. 👎 on this one.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Please revert this change @Lin-Buo-Ren

@@ -0,0 +1,3 @@
# /snap/patches
Copy link
Member

Choose a reason for hiding this comment

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

Files in /snap/ will be copied into the package. They cannot be filtered via stage: or prime: keywords. 👎 on this one.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Please revert this change @Lin-Buo-Ren

## with localestring format to let the translators fill in
## additional localized string and use these values to replace
## the Name keys here.
s/^\(Name\(\[.\+\]\)\?=.*\)$/\1 (Snappy Edition)/g
Copy link
Member

Choose a reason for hiding this comment

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

I don't like calling out that an application in a snap is any different to the same application packaged elsewhere. This is an example that people will use in their packages if we include it in this repo. I therefore think this line should be dropped.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Also Snappy is not how we refer to Snapcraft or Snaps these days. @Lin-Buo-Ren please revert this change.

# DISABLED: Bug #1775582 “`organize:{ /: another-dir/ }` causes the items under host root directory to be copied in another-dir” : Bugs : Snapcraft
# https://forum.snapcraft.io/t/organize-another-dir-causes-the-items-under-host-root-directory-to-be-copied-in-another-dir/5806
#organize:
#/: patches/
Copy link
Member

Choose a reason for hiding this comment

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

please drop the commented-out block

Copy link
Member

Choose a reason for hiding this comment

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

I agree, please remove this @Lin-Buo-Ren. I don't think we should be adding boilerplate that is only required for a few cases.

"$SNAPCRAFT_PART_INSTALL"/*.patch \
"$SNAPCRAFT_PART_INSTALL"/*.sed \
"$SNAPCRAFT_PART_INSTALL"/patches \
|| true # Empty patches folder is allowed
Copy link
Member

Choose a reason for hiding this comment

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

this override script isn't needed, please drop.

Copy link
Member

Choose a reason for hiding this comment

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

I really don't think we should be complicating a template yaml with patches. They are only required in rare cases.

|| true # Empty patches folder is allowed
stage:
- patches/*
override-prime: 'true'
Copy link
Member

Choose a reason for hiding this comment

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

replace with:

prime: [-*]

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see this patches stuff removed from the template yaml entirely. However, @Lin-Buo-Ren you have come up with an interesting use case. Perhaps a blog post on snapcraft.io about best practice for applying patches, when required, to a snap would be a good idea? What to collaborate on that?

"$SNAPCRAFT_PART_INSTALL"/patches \
|| true # Empty patches folder is allowed
stage:
- patches/*
Copy link
Member

Choose a reason for hiding this comment

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

This isn't needed.

@brlin-tw
Copy link
Contributor Author

brlin-tw commented Sep 25, 2018

Thanks for the review, I'll find some time to drop the undesirable content.

@merlijn-sebrechts
Copy link
Member

@Lin-Buo-Ren are you still working on this? I'll close it for the time being. Feel free to reopen it when you find time to work on it again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants