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.

Sign up for GitHub

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

Jump to bottom

cli: fix push on Windows #2922

Merged
merged 9 commits into from
May 21, 2020
Merged

cli: fix push on Windows #2922

merged 9 commits into from
May 21, 2020

Conversation

NickZ
Copy link

@NickZ NickZ commented Feb 8, 2020 •
edited
Loading

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh tests/unit?

Extracting the metadata didn't work on Windows, because it used os.path.join() and unsquashfs expected a Unix path rather than a Windows path

Fix push on Windows
994f016 
Extracting the metadata didn't work on Windows, because it used os.path.join() and unsquashfs expected a unix path rather than a Windows path
@NickZ NickZ requested a review from cjp256 February 8, 2020 23:57
@NickZ NickZ marked this pull request as ready for review February 9, 2020 06:28
@cjp256
Copy link
Contributor

cjp256 commented Feb 10, 2020 •
edited
Loading

How did you install unsquashfs on the host?

FWIW, I've been trying to use pathlib.Path and as_posix() to indicate when something requires posix formatted strings. And comment accordingly. Though now I realize i have been abusing it some and need to make some corrections..

I wonder how we should handle unsquashfs availability host for Windows...

@NickZ
Copy link
Author

NickZ commented Feb 10, 2020

I installed the cygwin version of unsquashfs; as far as I am aware, this is the only version of unsquashfs available for windows. I have the installation process laid out in my windows instructions here: https://gist.github.com/NickZ/d50d56348e2be7cd26fbecabdb02cb6f

cjp256
cjp256 previously approved these changes Feb 28, 2020
View reviewed changes
Copy link
Contributor

@cjp256 cjp256 left a comment

Choose a reason for hiding this comment

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

nit: comment punctuation (https://github.com/snapcore/snapcraft/blob/master/CODE_STYLE.md#comments) but otherwise LGTM, thanks!

@sergiusens sergiusens added the bug Actual bad behavior that don't fall into maintenance or documentation label May 13, 2020
sergiusens
sergiusens previously approved these changes May 13, 2020
View reviewed changes
snapcraft/internal/cache/_snap.py Outdated Show resolved Hide resolved
snapcraft/_store.py Outdated Show resolved Hide resolved
@sergiusens sergiusens dismissed stale reviews from cjp256 and themself via 4709176 May 13, 2020 11:13
cjp256
cjp256 approved these changes May 21, 2020
View reviewed changes
Copy link
Contributor

@cjp256 cjp256 left a comment

Choose a reason for hiding this comment

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

This assumes cygwin unsquashfs is available, which is good until we come up with a better solution. LGTM

@sergiusens sergiusens changed the title Fix push on Windows cli: fix push on Windows May 21, 2020
@sergiusens sergiusens merged commit fd1df20 into canonical:master May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjp256 cjp256 cjp256 approved these changes

@sergiusens sergiusens sergiusens left review comments

Assignees
No one assigned
Labels
bug Actual bad behavior that don't fall into maintenance or documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@NickZ @cjp256 @sergiusens