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

fix: strip extra fields out before creating snap.yaml (#7104) #7110

Merged
merged 2 commits into from
Sep 8, 2022

Conversation

MikeJerred
Copy link
Contributor

I have also removed the line TMPDIR: "$XDG_RUNTIME_DIR" as it causes problems with multiple instances (see the chromium bug: https://bugs.launchpad.net/ubuntu/+source/chromium-browser/+bug/1838508)

@changeset-bot
Copy link

changeset-bot bot commented Sep 1, 2022

🦋 Changeset detected

Latest commit: 71ff590

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
app-builder-lib Patch
dmg-builder Patch
electron-builder-squirrel-windows Patch
electron-builder Patch
electron-forge-maker-appimage Patch
electron-forge-maker-nsis-web Patch
electron-forge-maker-nsis Patch
electron-forge-maker-snap Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Sep 1, 2022

Deploy Preview for car-park-attendant-cleat-11576 ready!

Name Link
🔨 Latest commit 71ff590
🔍 Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/631a2198416aec00094eb2a6
😎 Deploy Preview https://deploy-preview-7110--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@mmaietta
Copy link
Collaborator

mmaietta commented Sep 1, 2022

Looks like test snapshots need to be updated using UPDATE_SNAPSHOT=true pnpm test-linux

Any thoughts on why this needs to be added to the test snapshot now? I don't recognize the code. Maybe that's why snap.parts was earlier in the logic 🤷

+       "override-build": "mkdir -p \"$SNAPCRAFT_PART_INSTALL/data-dir/themes\" mkdir -p \"$SNAPCRAFT_PART_INSTALL/data-dir/icons\" mkdir -p \"$SNAPCRAFT_PART_INSTALL/data-dir/sounds\" mkdir $SNAPCRAFT_PART_INSTALL/gnome-platform
    + ",
    +       "plugin": "nil",
    +     },
    +     "launch-scripts": Object {
    +       "plugin": "dump",
    +       "source": "scripts",
    +     },
    +   },

@MikeJerred
Copy link
Contributor Author

Looks like test snapshots need to be updated using UPDATE_SNAPSHOT=true pnpm test-linux

Any thoughts on why this needs to be added to the test snapshot now? I don't recognize the code. Maybe that's why snap.parts was earlier in the logic 🤷

+       "override-build": "mkdir -p \"$SNAPCRAFT_PART_INSTALL/data-dir/themes\" mkdir -p \"$SNAPCRAFT_PART_INSTALL/data-dir/icons\" mkdir -p \"$SNAPCRAFT_PART_INSTALL/data-dir/sounds\" mkdir $SNAPCRAFT_PART_INSTALL/gnome-platform
    + ",
    +       "plugin": "nil",
    +     },
    +     "launch-scripts": Object {
    +       "plugin": "dump",
    +       "source": "scripts",
    +     },
    +   },

Looks like effectiveOptionComputed should be taking the finished snap file, so I need to move my code before it.

@MikeJerred
Copy link
Contributor Author

I tried to run this but I get an error:

. prepare: fatal: detected dubious ownership in repository at '/project'
. prepare: To add an exception for this directory, call:
. prepare:      git config --global --add safe.directory /project

(running git config --global --add safe.directory /project makes no difference)

@mmaietta
Copy link
Collaborator

mmaietta commented Sep 2, 2022

Never seen that error before. Anyways, I pulled your PR locally and made some edits to add a unit test for template apps. :) Please take a look when you have a chance.
MikeJerred#1

@MikeJerred
Copy link
Contributor Author

@mmaietta I've updated this PR, but cannot generate the snapshots or check that the tests work

@mmaietta
Copy link
Collaborator

mmaietta commented Sep 6, 2022

@mmaietta I've updated this PR, but cannot generate the snapshots or check that the tests work

It seems that not all the changes from my PR were incorporated into your PR. Namely, the snapshot updates and the reinsert of delete snap.parts
https://github.com/MikeJerred/electron-builder/pull/1/files#diff-0354454e32db9abdc255b34f4188224fd0ea698bfa76355aa99b43ce64c3beb0

@MikeJerred
Copy link
Contributor Author

@mmaietta I've updated this PR, but cannot generate the snapshots or check that the tests work

It seems that not all the changes from my PR were incorporated into your PR. Namely, the snapshot updates and the reinsert of delete snap.parts https://github.com/MikeJerred/electron-builder/pull/1/files#diff-0354454e32db9abdc255b34f4188224fd0ea698bfa76355aa99b43ce64c3beb0

I changed the test to check for removal of snap.compression so I think the snapshot will need updating for that? Also I'm pretty sure the removal of snap.parts doesn't need to be moved up.

@mmaietta
Copy link
Collaborator

mmaietta commented Sep 6, 2022

Also I'm pretty sure the removal of snap.parts doesn't need to be moved up.

It does. Otherwise current snapshots break.

I'm able to run these tests with just TEST_FILES=snapTest UPDATE_SNAPSHOT=true pnpm test as opposed to having to use docker. Can you try that?

@MikeJerred
Copy link
Contributor Author

Also I'm pretty sure the removal of snap.parts doesn't need to be moved up.

It does. Otherwise current snapshots break.

I'm able to run these tests with just TEST_FILES=snapTest UPDATE_SNAPSHOT=true pnpm test as opposed to having to use docker. Can you try that?

Awesome! On Windows it skipped the test but using WSL this works :)
I've updated this PR with the new snapshots and fixed the existing compression test which was checking for the existence of snap.compression which has now been removed. Test suite is now passing.

test/src/linux/snapTest.ts Outdated Show resolved Hide resolved
@mmaietta mmaietta merged commit 0a7025e into electron-userland:master Sep 8, 2022
@github-actions github-actions bot mentioned this pull request Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snap.yaml has invalid fields
2 participants