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 CI for Windows #261

Merged
merged 7 commits into from
Apr 21, 2021
Merged

Add CI for Windows #261

merged 7 commits into from
Apr 21, 2021

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Apr 19, 2021

Closes #36.

Took inspiration from here.

The Windows run currently takes ~9 minutes.

While implementing this, I found the bug which caused some people asking in our channels why they got this error when building a contract under Windows:

[3/5] Optimizing wasm file
ERROR: The workspace root package should be a workspace member

It was due to a comparison of a non-canonicalized path with a canonicalized one.

workspace_root (non-canonicalized): "D:\\a\\cargo-contract\\cargo-contract\\foobar"
package_path (canonicalized):       "\\\\?\\D:\\a\\cargo-contract\\cargo-contract\\foobar"
comparison: Some("D:\\a\\cargo-contract\\cargo-contract\\foobar") == Some("\\\\?\\D:\\a\\cargo-contract\\cargo-contract\\foobar")

The \\\\? prefix appears only under Windows ‒ there a path with this prefix is the correct canonical path, this allows Windows-specific "long" paths (more info).

I'm not sure if there might be more places in our code which could cause issues with this path discrepancy, FWIW tests are green.

I can do a new release once we have merged this.

@cmichi cmichi force-pushed the cmichi-add-windows-build-ci-stage branch 17 times, most recently from 3dcc1e5 to c1fef66 Compare April 19, 2021 08:12
@cmichi cmichi force-pushed the cmichi-add-windows-build-ci-stage branch from c1fef66 to f31583f Compare April 19, 2021 08:15
@cmichi cmichi force-pushed the cmichi-add-windows-build-ci-stage branch from f31583f to c6c454f Compare April 19, 2021 08:24
@cmichi cmichi marked this pull request as ready for review April 19, 2021 08:34
@cmichi cmichi requested review from ascjones and TriplEight April 19, 2021 08:34
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM, CI stuff needs a double check from @TriplEight though.

.github/workflows/windows.yml Outdated Show resolved Hide resolved
@@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed
- Fixed `ERROR: The workspace root package should be a workspace member` when building a contract
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

@TriplEight TriplEight left a comment

Choose a reason for hiding this comment

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

LGTM.
You'll drastically improve caching efficiency if you'll check some of your duplicated dependencies with different versions and lead them to a common denominator.

@cmichi
Copy link
Collaborator Author

cmichi commented Apr 21, 2021

You'll drastically improve caching efficiency if you'll check some of your duplicated dependencies with different versions and lead them to a common denominator.

You mean the duplicates that show up if cargo tree -d is run? I recently looked through it, but it's third party dependencies which use different versions of the same lib.

@TriplEight
Copy link
Contributor

Yeah, kind of. Then there's nothing we can do :(

@cmichi cmichi merged commit 6774f34 into master Apr 21, 2021
@cmichi cmichi deleted the cmichi-add-windows-build-ci-stage branch April 21, 2021 14:53
@cmichi cmichi mentioned this pull request Apr 21, 2021
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.

Running on Windows
3 participants