-
Notifications
You must be signed in to change notification settings - Fork 63
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
polkavm-test-data #240
base: master
Are you sure you want to change the base?
polkavm-test-data #240
Conversation
I don't know how to fix build-and-test-windows. |
c5fe193
to
df084cf
Compare
The windows runner doesn't seem to use a rustup managed cargo. So they are not able to download and use the toolchain specified in the |
crates/polkavm-test-data/build.rs
Outdated
let mut cmd = Command::new("cargo"); | ||
|
||
cmd.current_dir(project_path) | ||
.env_clear() |
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.
Why the env_clear
?
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.
Not sure. I need refresh on Monday. It's a strong possibility that it is for no good reason ;-)
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 history for this is that I derived the code from substrate/frame/revive/fixtures/build.rs.
I guess the argument here (and fixtures) is that by doing this we remove any side-effects of environment variables set by parent i.e. always starting from the same state.
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.
I recall @athei did the code I used as basis so maybe he has a comment for this...
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.
Yeah it makes sense since we don't want other variables meant for other targets to influence this build.
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.
See my remarks below.
This is weird; it's clearly using rustup because
but when it's ran from inside of the I'm not sure how to fix it; you'll have to fiddle with it. Maybe clearing the environment variables screws it up? |
Oops didn't see that and jumped to conclusions. Rustup seems to installed and is picking up the toolchain file in the repo root.
Yes it is weird. Maybe some platform differences regarding pathes?
We do the clearing in the other build scripts in polkadot-sdk, too. But I don't think it is ever tested on Windows. Yeah maybe removing the clearing and work from there. Maybe additional or different variables need to be forwarded on Windows. |
32a763d
to
f7481c7
Compare
At least now it is failing everywhere? :D |
Working on it... temporary glitch ;---) |
e5b98a7
to
c8f485b
Compare
@koute, @athei, I'll go a few diff's now that I've fixed the stuff I know how to fix! Diff 1:
Result:
Diff 2:
Result:
Diff 3:
Result:
How should I move forward? |
Debug this further. Probably reproducing in a Windows VM. Looking at the code nothing obvious comes to mind for me. |
I'm working on setting up a Windows VM. It is worth of investment as Windows could break also in future because POSIX does not always align that well in that environment. So yeah, we're on the same page. |
I spent a lot of time on setting up a Windows environment that is usable. Now I have an image that logins automatically and boots to the terminal, and I can reach the host via ssh. Also environment has been setup. This what happens: @athei, @koute: So I guess this maps to replacing |
I think it was useful spend couple of days to figure out effective way to use for Windows. After this I can reproduce bugs in no time. It was an investment ;-) |
3951732
to
2c921da
Compare
Without this Linux build will act the same (will complain about -Z flags):
So better to have this initial environment setup branched and find the right recipe for Windows. |
1a7d5c0
to
85fe529
Compare
@athei, @koute, possible success I got into brand new error:
I removed It is very promising given that it is now going after nightly and not complaining about -Z. |
bc61f52
to
5022f31
Compare
Signed-off-by: Jarkko Sakkinen <[email protected]>
5022f31
to
302c305
Compare
Better to take a break with this one, nothing comes to mind ATM to try out with Windows build. Revisiting when new idea pop up. |
Closes: #209