Skip to content

/ saltfs

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 building on Windows #331

Merged
merged 1 commit into from Apr 25, 2016
Merged

Fix building on Windows #331

merged 1 commit into from Apr 25, 2016

Conversation

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Apr 21, 2016

r? @edunham

This appears to fix the Windows build, when I've tested the environment vars and the command via RDP to the build machine. Here's hoping!

This change is Reviewable
@aneeshusa
Copy link
Member

aneeshusa commented Apr 21, 2016

It may be more useful to put this in a .bashrc file instead, so it doesn't need to be repeated for every command.
@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Apr 21, 2016

Put the cd into .bashrc? I'm not sure I'd want to do that, as at some point we should have windows-dev and windows-rel builders (and possibly split for different tests), each of which would have a different buildbot directory.
@aneeshusa
Copy link
Member

aneeshusa commented Apr 22, 2016

OK, that makes sense (we could do things with environment variables and .bashrc but that sounds like a bad idea). You might want to make a small function to build a full windows command array from just the './mach build' string of interest to avoid hitting the line length limit.
@larsbergstrom larsbergstrom force-pushed the larsbergstrom:add_paths_windows branch from cce4714 to 8ffe161 Apr 22, 2016
@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Apr 22, 2016

Is the changed patch more of what you're looking for?
buildbot/master/files/config/factories.py Outdated
windows = ServoFactory([
# TODO: convert this to use DynamicServoFactory
# We need to run each command in a bash login shell, which breaks the
# heuristics used by DynamicServoFactory.make_step
steps.Compile(command=["bash", "-l", "-c", "./mach build -d -v"],
steps.Compile(command=["bash", "-l", "-c", ""], make_cd_string("./mach build -d -v"),

This comment has been minimized.

Sign in to view
@jdm

jdm Apr 22, 2016

Member

This looks wrong compared to the next change.

This comment has been minimized.

Sign in to view
@larsbergstrom

larsbergstrom Apr 22, 2016

Author Contributor

Good catch! I hate that it is basically impossible to test any of this locally.

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Apr 25, 2016

Member

Actually, this could have been caught via ./test.py sls.buildbot.master inside Vagrant - there's some extra setup that's required but I've been using it and it's pretty helpful. I'm trying to add local tests where I can :)
@larsbergstrom larsbergstrom force-pushed the larsbergstrom:add_paths_windows branch from 8ffe161 to ffff6bb Apr 22, 2016
@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Apr 22, 2016

That should hopefully be less worse.
@larsbergstrom
Fix building on Windows
Loading status checks…
8c40eda
@larsbergstrom larsbergstrom force-pushed the larsbergstrom:add_paths_windows branch from ffff6bb to 8c40eda Apr 22, 2016
@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Apr 22, 2016

OK, it even passes Travis now :-)
@aneeshusa
Copy link
Member

aneeshusa commented Apr 25, 2016

This looks OK, but I was thinking that the function could create the entire

["bash", "-l", "-c", "cd /c/buildbot/slave/windows/build; ./mach build --dev --verbose"]

array, instead of just the command at the end.
@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Apr 25, 2016

@bors-servo r=aneeshusa

Sounds good; I will open an E-Easy to make that change.
@bors-servo
Copy link
Contributor

bors-servo commented Apr 25, 2016

📌 Commit 8c40eda has been approved by aneeshusa
@bors-servo
Copy link
Contributor

bors-servo commented Apr 25, 2016

⌛ Testing commit 8c40eda with merge 836ce58...
bors-servo added a commit that referenced this pull request Apr 25, 2016
@bors-servo
Auto merge of #331 - larsbergstrom:add_paths_windows, r=aneeshusa
Loading status checks…
836ce58 
Fix building on Windows

r? @edunham

This *appears* to fix the Windows build, when I've tested the environment vars and the command via RDP to the build machine. Here's hoping!

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/331)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 25, 2016

☀️ Test successful - travis
@bors-servo bors-servo merged commit 8c40eda into servo:master Apr 25, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@larsbergstrom larsbergstrom mentioned this pull request Apr 25, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@larsbergstrom @aneeshusa @bors-servo @jdm
You can’t perform that action at this time.