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

gnome extension #2398

Open
wants to merge 57 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@diddledan
Copy link
Contributor

diddledan commented Nov 8, 2018

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
  • If this is a new feature. Have you discussed the design on the forum?

I discussed this with @kyrofa directly at snapcraft summit.

  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh tests/unit?

I have successfully run unit and spread tests relating to this feature.


Please review with prejudice :-)

@diddledan diddledan force-pushed the diddledan:desktop-gnome-extension branch from 1aee0fe to bf68218 Nov 8, 2018

@diddledan

This comment has been minimized.

Copy link
Contributor Author

diddledan commented Nov 8, 2018

spread test is failing, but the failure is in unrelated code (maven plugin) to this pull request. I blame @sergiusens ! :-p

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 11, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@636d6be). Click here to learn what that means.
The diff coverage is 89.18%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2398   +/-   ##
=========================================
  Coverage          ?   89.87%           
=========================================
  Files             ?      200           
  Lines             ?    13377           
  Branches          ?     2024           
=========================================
  Hits              ?    12023           
  Misses            ?      926           
  Partials          ?      428
Impacted Files Coverage Δ
...raft/internal/project_loader/_extensions/_utils.py 97.84% <100%> (ø)
...rnal/project_loader/_extensions/_desktop_common.py 100% <100%> (ø)
.../internal/project_loader/_extensions/gnome-3-28.py 84% <84%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 636d6be...723257d. Read the comment docs.

@sergiusens sergiusens requested a review from niemeyer Nov 14, 2018

@kyrofa

This comment has been minimized.

Copy link
Member

kyrofa commented Nov 14, 2018

I think it's important for us to start small, here. Rather than simply turning the desktop helper into an extension, we should (at least somewhat) cleanroom it. Create a snap of a few gnome applications with no remote parts or extensions, and see what common bits fall out.

@kenvandine

This comment has been minimized.

Copy link

kenvandine commented Nov 20, 2018

This extension should also connect to the platform snap's content interface. Ideally, for core16 connect to gnome-3-26-1604 and for core18 connect to gnome-3-28-1804.

diddledan added some commits Jan 7, 2019

refactor gnome extension
Move most of the environment variables into snapcraft.yaml
merge desktop extension script
combine desktop-init and desktop-exports into a single script
fix references to content interfaces paths
also ensure XCURSOR_PATH is correct
add shared snap mountpoints and update launcher
* ensure the shared data snap mountpoints exist
* update launcher scripts with fixes to ensure gsettings schemas are
correctly compiled.

@diddledan diddledan force-pushed the diddledan:desktop-gnome-extension branch from 03bd975 to 54215e3 Jan 7, 2019

squash test errors
code style errors are fixed

@diddledan diddledan force-pushed the diddledan:desktop-gnome-extension branch from d7c71ed to ae2e4c5 Jan 8, 2019

diddledan added some commits Jan 8, 2019

fix GIO modules symlinks; update gnome-extension
* fix incorrect quoting of GIO modules wildcard when creating symlink
* add better coverage of environment variables in base desktop extension
and overrides in gnome-extension
add layout to fix libwebkitgtk
libwebkitgtk hard-codes it's runtime executable helper path so we add a
layout definition for it.
Show resolved Hide resolved extensions/desktop-common/desktop-init Outdated
@oSoMoN
Copy link
Contributor

oSoMoN left a comment

I modified the gnome-calculator snap's snapcraft.yaml to use the extension (and used the snapcraft-pr snap to conveniently test it), and got some early schema validation errors, see comments inline.

@kenvandine

This comment has been minimized.

Copy link

kenvandine commented Jan 10, 2019

If I include the environment keyword in my yaml with LD_LIBRARY_PATH it will override the LD_LIBRARY_PATH set by the gnome extension yaml. This wasn't an issue when the desktop-launch script would set these variables. I think it would be best to move LD_LIBRARY_PATH (and other runtime variables) to the command-chain.

diddledan added some commits Jan 14, 2019

set environment in command-chain scripts
testing the gnome extension lead to missing environment variable
settings where the extension environment settings were being overridden
by user-defined variables. To manage this we are reverting to setting
the extension environment in the scripts included in the command-chain.
@oSoMoN

This comment has been minimized.

Copy link
Contributor

oSoMoN commented Feb 4, 2019

+1 from me. This is a solid basis, successfully tested by rebuilding a bunch of non-trivial snaps with it. I think we should merge it, if there are remaining issues we will find them and fix them as we rebuild more snaps with the extension.

@kenvandine

This comment has been minimized.

Copy link

kenvandine commented Feb 12, 2019

Something else the extension should provide a layout for, iso-codes

  • layout:
  • /usr/share/xml/iso-codes:
  •  symlink: $SNAP/gnome-platform/usr/share/xml/iso-codes
    
Add iso-codes layout to gnome extension
Signed-off-by: Daniel Llewellyn <daniel@bowlhat.net>

@diddledan diddledan force-pushed the diddledan:desktop-gnome-extension branch from d409ad6 to cdafbe2 Feb 12, 2019

@diddledan

This comment has been minimized.

Copy link
Contributor Author

diddledan commented Feb 12, 2019

Ken: I've added that layout definition and updated the branch to match master.
Sergio: it looks like travis failed on the CLA check after my merge of master pulling in every user ever some of whom seem to have not signed the CLA.

@sergiusens

This comment has been minimized.

Copy link
Collaborator

sergiusens commented Feb 13, 2019

I think you confused the light weight system by force pushing followed by a merge

@diddledan

This comment has been minimized.

Copy link
Contributor Author

diddledan commented Feb 13, 2019

force pushing followed by a merge

I like doing things weirdly :-p

@kenvandine

This comment has been minimized.

Copy link

kenvandine commented Feb 14, 2019

Before we merge this, we need to have a discussion about handling different versions of the gnome platform. We're going to need a way for snap packagers to use newer versions of libraries. This will result in newer platform snaps. For example we'll probably end up with a gnome-3-32-1804 platform snap soon that includes newer glib, gtk, etc.

Perhaps we should name the extension accordingly? gnome-3-28-1084 and later add more as we have new platform snaps available, always matching the platform snap name?

@cmatsuoka

This comment has been minimized.

Copy link
Collaborator

cmatsuoka commented Feb 14, 2019

Also it should be discussed what happens when a new base is available and the user tries to build on one base when the platform was built with a different base.

Note that this extension does not support classically-confined snaps at this time.
"""

supported_bases = ("core16", "core18")

This comment has been minimized.

@sergiusens

sergiusens Feb 18, 2019

Collaborator

Potentially missing feature
supported_architectures = ....

sergiusens added some commits Feb 19, 2019

ci: shallow clones for CLA checks on travis
Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
@kenvandine

This comment has been minimized.

Copy link

kenvandine commented Feb 19, 2019

Before we merge this, we need to have a discussion about handling different versions of the gnome platform. We're going to need a way for snap packagers to use newer versions of libraries. This will result in newer platform snaps. For example we'll probably end up with a gnome-3-32-1804 platform snap soon that includes newer glib, gtk, etc.

Perhaps we should name the extension accordingly? gnome-3-28-1084 and later add more as we have new platform snaps available, always matching the platform snap name?

As discussed this week, we will rename this to gnome-3-28 and infer the platform version from the base. For example, if base is core18 we'll connect to the gnome-3-28-1804 platform snap. In the future, we'll add support for additional gnome versions like gnome-3-32 which would connect to gnome-3-32-1804. This also allows us to handle other bases, like fedora perhaps which could use a different platform snap or even just stage packages.

We also discussed dropping support for core, only supporting core18 from the start. If there is enough demand we can discuss adding support for core.

rename gnome extension to gnome-3-28
drop core support from gnome-3-28 extension
rename gnome extension to gnome-3-28

Signed-off-by: Daniel Llewellyn <daniel@bowlhat.net>

@diddledan diddledan force-pushed the diddledan:desktop-gnome-extension branch from d8fe6cc to 5a2ea43 Feb 20, 2019

Minor syntax errors on gnome-3-28 extension
Signed-off-by: Daniel Llewellyn <daniel@bowlhat.net>

@diddledan diddledan force-pushed the diddledan:desktop-gnome-extension branch from 5a2ea43 to e1361d0 Feb 20, 2019

diddledan and others added some commits Feb 20, 2019

incorrect quote style in extensions/_utils.py
Signed-off-by: Daniel Llewellyn <daniel@bowlhat.net>
force supported_cores to be a tuple in gnome ext
Signed-off-by: Daniel Llewellyn <daniel@bowlhat.net>

# On Fedora $SNAP is under /var and there is some magic to map it to /snap.
# # We need to handle that case and reset $SNAP
SNAP="${SNAP/\/var\/lib\/snapd/}"

This comment has been minimized.

@sergiusens

sergiusens Feb 22, 2019

Collaborator

Can we get a better code comment for this?
We need to test this on Fedora

@oSoMoN

This comment has been minimized.

Copy link
Contributor

oSoMoN commented Feb 22, 2019

I successfully rebuilt the chromium snap with this extension and published it under the candidate/gnome-3-28-extension channel (amd64 only).
Successfully tested on Ubuntu 18.04 and Fedora 28.

@oSoMoN

This comment has been minimized.

Copy link
Contributor

oSoMoN commented Mar 15, 2019

@diddledan please consider backporting ubuntu/snapcraft-desktop-helpers#176.

diddledan added some commits Mar 15, 2019

Backport ubuntu/snapcraft-desktop-helpers#176
Add `$ARCH` triplet support for ppc64el architecture.

Signed-off-by: Daniel Llewellyn <daniel@bowlhat.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.