-
Notifications
You must be signed in to change notification settings - Fork 16
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: Fix Windows zip file #816
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So given that all docs say to explicitly use forward slashes, I'm confused whether this change actually fixes the issue. Kind of seems like it could be making things worse? How did you test this?
Given this node_modules
:
node_modules
└── @saucelabs
└── testcafe-reporter-saucelabs
On Windows, before your change, the archived directory would be node_modules\@saucelabs/testcafe-reporter-saucelabs
(note the last separator is a /
) since archive/zip
is using path.Join
instead of filepath.Join
. And when unarchiving on Mac we would see this hierarchy (which matches what I see in the jira ticket):
└── node_modules
└── node_modules\@saucelabs
└── testcafe-reporter-saucelabs
But after this change, the archived directory would be node_modules\@saucelabs\testcafe-reporter-saucelabs
which, I think, would be unarchived like this on Mac? (I dunno I can't test this):
└── node_modules
└── node_modules\@saucelabs
└── node_modules\@saucelabs\testcafe-reporter-saucelabs
Makes me think maybe all we need here is filepath.ToSlash
to make sure the path we give to Create
always has forward slashes, regardless of platform... (https://pkg.go.dev/path/filepath#ToSlash)
internal/saucecloud/zip/archive.go
Outdated
@@ -71,7 +72,7 @@ func ArchiveFiles(targetFileName string, targetDir string, sourceDir string, fil | |||
if err != nil { | |||
return "", err | |||
} | |||
fileCount, length, err := z.Add(f, rel) | |||
fileCount, length, err := z.Add(f, filepath.ToSlash(rel)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The root cause is
filepath.Rel(sourceDir, filepath.Dir(f))
returns node_modules\@cucumber
on Windows.
Proposed changes
When triggering tests on a Mac platform from a Windows machine, there is an issue with unzipping packages that have an
@
prefix in the node_modules zip.Passed job: https://app.saucelabs.com/tests/979a262f57724f80b45de70392039333
After:
Types of changes
Checklist
Further comments