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

cross-test on a Windows host #1251

Merged
merged 7 commits into from
Mar 24, 2020
Merged

cross-test on a Windows host #1251

merged 7 commits into from
Mar 24, 2020

Conversation

RalfJung
Copy link
Member

No description provided.

@RalfJung
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Mar 22, 2020

⌛ Trying commit 6db3c36 with merge e148c89...

bors added a commit that referenced this pull request Mar 22, 2020
cross-test on a Windows host
@bors
Copy link
Contributor

bors commented Mar 22, 2020

💔 Test failed - status-appveyor

@RalfJung
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Mar 22, 2020

⌛ Trying commit c7f8fed with merge aeefbb9...

bors added a commit that referenced this pull request Mar 22, 2020
cross-test on a Windows host
@bors
Copy link
Contributor

bors commented Mar 22, 2020

💔 Test failed - status-appveyor

@RalfJung
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Mar 22, 2020

⌛ Trying commit 0325dd5 with merge 0db1dcd...

bors added a commit that referenced this pull request Mar 22, 2020
cross-test on a Windows host
@bors
Copy link
Contributor

bors commented Mar 22, 2020

💔 Test failed - status-appveyor

@RalfJung
Copy link
Member Author

@CAD97 since you seem to use Windows, do you have any idea how to unset an environment variable in a batch script?

Somehow, batch scripts is an even worse language than bash scripts, and that really should be impossible.^^ There's a hole lot of code duplication here, but I don't dare even try factoring that into a function.

@RalfJung
Copy link
Member Author

Also Cc @JOE1994

.appveyor.yml Outdated
- cd test-cargo-miri
- '"C:\msys64\mingw64\bin\python3.exe" run-test.py'
- cd ..
- set MIRI_SYSROOT=
Copy link
Member Author

Choose a reason for hiding this comment

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

This here seems to not have the effect of deleting the env var, even though that's what the internet says I should do.

@CAD97
Copy link

CAD97 commented Mar 22, 2020

In CMD, set VAR= is in fact the way to "undefine" an environment variable. Likely what's happening is that the executed command includes trailing whitespace for some reason; set "VAR=" should prevent this being an issue.

C:\Users\CAD>if defined demo ( echo defined )

C:\Users\CAD>if not defined demo ( echo not defined )
not defined

C:\Users\CAD>set demo=demo

C:\Users\CAD>if defined demo ( echo defined )
defined

C:\Users\CAD>if not defined demo ( echo not defined )

C:\Users\CAD>set demo=

C:\Users\CAD>if defined demo ( echo defined )

C:\Users\CAD>if not defined demo ( echo not defined )
not defined

C:\Users\CAD>set demo=␣

C:\Users\CAD>if defined demo ( echo defined )
defined

C:\Users\CAD>set "demo="␣

C:\Users\CAD>if not defined demo ( echo not defined )
not defined

I typically avoid command line scripting though, and I stick to Powershell when I have to. (And typically, I try to treat "empty string" as unset because of this exact issue where unsetting a variable often times just sets it to an empty string, but it's still set.)

@RalfJung
Copy link
Member Author

In CMD, set VAR= is in fact the way to "undefine" an environment variable. Likely what's happening is that the executed command includes trailing whitespace for some reason; set "VAR=" should prevent this being an issue.

I'll try that, thanks!

I typically avoid command line scripting though, and I stick to Powershell when I have to

Is there a way to do that with our AppVeyor scripting?

(And typically, I try to treat "empty string" as unset because of this exact issue where unsetting a variable often times just sets it to an empty string, but it's still set.)

I'd rather not change the platform-independent Rust part because of the idiosyncrasies of a 30 year old scripting language...

@RalfJung
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Mar 22, 2020

⌛ Trying commit 908f017 with merge c96442c...

bors added a commit that referenced this pull request Mar 22, 2020
cross-test on a Windows host
@bors
Copy link
Contributor

bors commented Mar 22, 2020

💔 Test failed - status-appveyor

@RalfJung
Copy link
Member Author

It still says

[00:08:24] WARNING: MIRI_SYSROOT already set, not doing anything.

So, this hasn't worked. :/

@CAD97
Copy link

CAD97 commented Mar 22, 2020

Ok, so here's something else to try:

https://www.appveyor.com/docs/build-configuration/#interpreters-and-scripts: you can use powershell scripts via - ps: Powershell-Command.

In powershell, the syntax would be

PS C:\Users\CAD> Test-Path env:Demo
False
PS C:\Users\CAD> $env:Demo="demo"
PS C:\Users\CAD> Test-Path env:Demo
True
PS C:\Users\CAD> $env:Demo=$null
PS C:\Users\CAD> Test-Path env:Demo
False

@RalfJung
Copy link
Member Author

But then I would have to port the entire thing to PowerShell, right? I never used that language and can't test this script locally, so I don't think that would work.

I was going to try and see if I can't use a bash script on Windows instead -- maybe AppVeyor has git-for-Windows installed which, from what I hear, comes with a bash. Then we could even share some code with the Travis side of things.

If that doesn't work, I think someone else would have to rewrite this for PowerShell.

@JOE1994
Copy link
Contributor

JOE1994 commented Mar 23, 2020

But then I would have to port the entire thing to PowerShell, right? I never used that language and can't test this script locally, so I don't think that would work.

appVeyor allows us to choose between Command scripting and PowerShell scripting.

Replacing the lines - set "MIRI_SYSROOT=" to - ps: $env:MIRI_SYSROOT = "" solved the issue;
environment variable gets undefined properly. (Link to appveyor build of my fork).

p.s. replacing the lines - set "MIRI_SYSROOT=" to - cmd: set "MIRI_SYSROOT="doesn't solve the issue. (Link to another appveyor build of my fork)

Copy link
Contributor

@JOE1994 JOE1994 left a comment

Choose a reason for hiding this comment

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

OOPS... please disregard this review..

.appveyor.yml Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member Author

Looking good, isn't it? :D
@bors r+

@bors
Copy link
Contributor

bors commented Mar 24, 2020

📌 Commit e9e04e5 has been approved by RalfJung

bors added a commit that referenced this pull request Mar 24, 2020
@bors
Copy link
Contributor

bors commented Mar 24, 2020

⌛ Testing commit e9e04e5 with merge 6a21e5d...

@RalfJung
Copy link
Member Author

Looks like travis has trouble reporting the status back? CI is green but bors/GH do not notice...

@RalfJung
Copy link
Member Author

@bors retry

@bors
Copy link
Contributor

bors commented Mar 24, 2020

⌛ Testing commit e9e04e5 with merge 38d184c...

bors added a commit that referenced this pull request Mar 24, 2020
@RalfJung
Copy link
Member Author

@bors retry

@bors
Copy link
Contributor

bors commented Mar 24, 2020

⌛ Testing commit e9e04e5 with merge 71ae2a4...

bors added a commit that referenced this pull request Mar 24, 2020
@RalfJung
Copy link
Member Author

@bors retry

bors added a commit that referenced this pull request Mar 24, 2020
@bors
Copy link
Contributor

bors commented Mar 24, 2020

⌛ Testing commit e9e04e5 with merge 76d5bce...

@RalfJung
Copy link
Member Author

@bors retry

@bors
Copy link
Contributor

bors commented Mar 24, 2020

⌛ Testing commit e9e04e5 with merge 9b58440...

bors added a commit that referenced this pull request Mar 24, 2020
@RalfJung
Copy link
Member Author

@bors retry

@bors
Copy link
Contributor

bors commented Mar 24, 2020

⌛ Testing commit e9e04e5 with merge cefe09f...

bors added a commit that referenced this pull request Mar 24, 2020
@RalfJung
Copy link
Member Author

https://www.traviscistatus.com/ claims the issue is gone... let's see about that.
@bors retry

@bors
Copy link
Contributor

bors commented Mar 24, 2020

⌛ Testing commit e9e04e5 with merge a6d74a6...

@bors
Copy link
Contributor

bors commented Mar 24, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing a6d74a6 to master...

@bors bors merged commit a6d74a6 into rust-lang:master Mar 24, 2020
@RalfJung RalfJung deleted the win-cross branch March 24, 2020 13:37
@RalfJung
Copy link
Member Author

Hm, I just realized I should probably have waited for some feedback here... @oli-obk with this PR, the Miri shims that take file system paths replace / by \ or vice versa when host and target OS directory separators differ (e.g. when executing --target i686-unknown-linux-gnu on a Windows host). This is the only good way I could think of to actually make things work, because the Linux target methods obviously expect / as directory separator. Does that seem reasonable?

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.

4 participants