Skip to content

[RFC] Send output of building setup to stderr (fixes #1410) #1423

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.

Sign up for GitHub

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

Jump to bottom
Merged
mgsloan merged 1 commit into from
Nov 30, 2015
Merged

[RFC] Send output of building setup to stderr (fixes #1410) #1423

mgsloan merged 1 commit into from
Nov 30, 2015

Conversation

@rvion
Copy link
Contributor

@rvion rvion commented Nov 24, 2015

this PR

  • rename runIn to runCmd
  • add a new runCmd' function taking and extra (CreateProcess -> CreateProcess) param
  • fixes Send output of building Setup.hs to stderr #1410 by using runCmd' (\cp -> cp { std_out = UseHandle stderr }) in getSetupExe
  • define a new data CMD = CMD directoryToRunIn commandToRun envOverride commandLineArguments
  • refactor runIn, callProcess, callProcess' to take a CMD arg to ease code exploration, and argument passing

@rvion
Copy link
Contributor Author

rvion commented Nov 24, 2015

can someone review 5f9c75e ? I'm unsure about it.
I like it but I have no certitudes since it's my first dive into stack code

@rvion
Copy link
Contributor Author

rvion commented Nov 24, 2015

one of my motivations to refactor runIn, callProcess, callProcess' was that they were taking arguments in different order, and that it was harder to explore the code without a common CMD prefix.

@rvion rvion changed the title Send output of building setup 1410 [RFC] Send output of building setup 1410 Nov 24, 2015
@rvion rvion changed the title [RFC] Send output of building setup 1410 [RFC] Send output of building setup to stderr 1410 Nov 24, 2015
@rvion rvion changed the title [RFC] Send output of building setup to stderr 1410 [RFC] Send output of building setup to stderr (fixes #1410) Nov 24, 2015
@mgsloan
Copy link
Contributor

mgsloan commented Nov 25, 2015

5f9c75e seems good to me. I have the following nitpicks:

  • It'd be better to move the CMD type into System.Process.Run
  • Personally, I prefer camel-case acronyms, like Cmd. Not that this style guide is authoritative

The other commits look good too, thanks for working on this!

For some reason the build is failing in the windows CI: https://ci.appveyor.com/project/snoyberg/stack/build/1.0.546#L766

@rvion rvion force-pushed the Send-output-of-building-Setup-1410 branch 3 times, most recently from 43293f5 to 5c7ba6e Compare November 25, 2015 09:36
@rvion
Copy link
Contributor Author

rvion commented Nov 25, 2015

@mgsloan I rebased it cleanly on master, and tweaked it according to your comments

@borsboom
Copy link
Contributor

borsboom commented Nov 25, 2015

Looks like you might have accidentally added test.dot and test.html to the root in 3d49b6c.

It's failing to build on Windows because there is some CPP in Stack/Exec.hs to use callProcess on Windows, since it doesn't support the exec* system calls.

@rvion rvion mentioned this pull request Nov 25, 2015
Closed
@rvion
Copy link
Contributor Author

rvion commented Nov 25, 2015

@borsboom thanks
I think I've fixed those

@mgsloan
Copy link
Contributor

mgsloan commented Nov 26, 2015

Changes look good to me, One last thing before merging:

Can you please use git rebase --interactive to squash the test.dot / test.html removal commit atop 3d49b6c? Then, force push your branch. Thanks!

@rvion
fix commercialhaskell#1410 and refactor runIn, callProcess, etc. to t…
332636b 
…ake a Cmd arg

introduce CMD data type
add runCmd' following callProcess' model
actually fixes commercialhaskell#1410
rename CMD -> Cmd
move Cmd to System.Process.Run
clean redundant imports
remove test.dot and test.html
fix callProcess call on windows in Stack.Exec
@rvion rvion force-pushed the Send-output-of-building-Setup-1410 branch from 86189e4 to 332636b Compare November 29, 2015 08:53
@rvion
Copy link
Contributor Author

rvion commented Nov 29, 2015

I've squashed it all on one single commit

@mgsloan
Copy link
Contributor

mgsloan commented Nov 30, 2015

Thanks!

mgsloan added a commit that referenced this pull request Nov 30, 2015
@mgsloan
Merge pull request #1423 from rvion/Send-output-of-building-Setup-1410
8c02285 
[RFC] Send output of building setup to stderr (fixes #1410)
@mgsloan mgsloan merged commit 8c02285 into commercialhaskell:master Nov 30, 2015
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

Development

Successfully merging this pull request may close these issues.

Send output of building Setup.hs to stderr

3 participants

@rvion @mgsloan @borsboom