Use cross-spawn for robust, platform-independent spawning#50
Closed
raphinesse wants to merge 2 commits intomasterfrom
Closed
Use cross-spawn for robust, platform-independent spawning#50raphinesse wants to merge 2 commits intomasterfrom
cross-spawn for robust, platform-independent spawning#50raphinesse wants to merge 2 commits intomasterfrom
Conversation
janpio
reviewed
Sep 16, 2018
| } | ||
|
|
||
| /** | ||
| * A special implementation for child_process.spawn that handles |
Member
There was a problem hiding this comment.
could update that line now as well as child_process.spawn is not used any more directly
Contributor
Author
There was a problem hiding this comment.
IMHO, the wording is a bit unfortunate to begin with. Do you have a suggestion on how to improve it?
Member
There was a problem hiding this comment.
Wrapper for
crossSpawn.spawnthat makes surecmdis executable.
I don't really understand what else it does tbh.
Contributor
Author
There was a problem hiding this comment.
Frankly, I would not even mention cross-spawn. I'ts an implementation detail. So maybe we should just change The beginning to
A
child_process.spawn-compatible function that handles ...
Anyway, I think that's the least important part of this change 😅
Member
There was a problem hiding this comment.
It was the only one I could comment on without actually doing any work :D
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3 tasks
Member
|
I'm getting failures in the cordova-android, cordova-ios, and cordova-osx tests that I suspect are related to those tests not being updated to account for fs-extra in cordova-common and cordova-lib. I'm getting other random-seeming failures too: cordova-common resultscordova-lib resultscordova-android results |
Member
|
Yet cordova-common tests pass locally when run directly, both on master and on this branch. 🤷♂️ |
Contributor
Author
|
Thanks for your feedback @dpogue. Which branch of cordova-common were you using for the test run your first comment refers to? It was run on macOS, right? But yeah, I also had some weird test failure (using all the master branches) that did not occur when running the tests directly. That's why |
Member
|
Yes, this was all on macOS. The error logs are from running your test project pointing to the cross-spawn branch. |
Contributor
Author
|
Ah. So with master, everything passes? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Member
|
I |
Member
|
I tried to follow the instructions at the test repo, but had some problems - so I created raphinesse/cordova-cross-spawn-test#1 and raphinesse/cordova-cross-spawn-test#2. |
raphinesse
force-pushed
the
cross-spawn
branch
from
67b60b1 to
7100af0
Compare
Codecov Report
@@ Coverage Diff @@
## master #50 +/- ##
==========================================
+ Coverage 87.82% 88.71% +0.88%
==========================================
Files 19 19
Lines 1799 1772 -27
Branches 373 364 -9
==========================================
- Hits 1580 1572 -8
+ Misses 219 200 -19
Continue to review full report at Codecov.
|
Contributor
Author
|
@dpogue would you mind to run the |
33 tasks
Member
|
On Linux (both before and after): iOS, macOS/OSX, and Windows can't be run on Linux. On Mac, I'm still getting failures for cordova-common, cordova-lib, cordova-ios, and cordova-android even before I switch branches :( |
Contributor
Author
|
Thanks for trying again @dpogue. Unfortunately I had messed up the change and the tests did not actually run in series. I updated I also ran the test suite on Travis' OS X and updated the checklist in my original post accordingly. |
Contributor
Author
|
After the latest fixes for |
Contributor
Author
|
Extending the macOS tests on Travis CI to Judging from how the output looks, I'd aggree with @dpogue's previous suspicion:
|
Member
|
Okay, so the cordova-ios tests are known to fail with those errors. My cordova-ios and cordova-android tests also failed due to updated native toolchains. I got some coworkers to run it, and cordova-android is passing for them on macOS both before/after the cross-spawn changes. 😄 |
Member
|
I guess when we merge we should cherry-pick the cross-spawn commit and not the TMP one? |
Contributor
Author
|
@dpogue Exactly, the one marked TMP was only to silence an unrelated error. |
5 tasks
This was referenced Jul 12, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR removes any special treatment of Windows from
superspawnand instead delegates this work tocross-spawn. The interface ofsuperspawnis not changed. The goal behind this change is to reduce maintenance and improve cross-platform operability.This PR depends on apache/cordova-lib#622 (thanks @AlmirKadric) and closes apache/cordova-lib#688.
What testing has been done on this change?
cordova-commonpasscordova-browser)cordova-clitests passcordova-plugmantests passcordova-createtests passcordova-fetchtests passcordova-libtests passcordova-browsertests passcordova-androidtests passcordova-iostests passcordova-osxtests passcordova-windowstests passcordova-browser)cordova-clitests passcordova-plugmantests passcordova-createtests passcordova-fetchtests passcordova-libtests passcordova-browsertests passcordova-androidtests passcordova-iostests passcordova-osxtests passcordova-windowstests passcordova-browser)cordova-clitests passcordova-plugmantests passcordova-createtests passcordova-fetchtests passcordova-libtests passcordova-browsertests passcordova-androidtests passcordova-iostests passcordova-osxtests passcordova-windowstests passAs you might have noticed, I still need some help to tick off all those boxes. Especially for all platforms that require an SDK to be set-up. So I'd like to ask anyone who has one of the missing combinations at hand: please check out the test repository I prepared, follow the instructions in the README and share your results here. Running the tests is easy as pie, I promise 😄.
Please run as much of the tests as you can, even if some of the boxes are already ticked off. This is especially true if you are on Windows and even more so if you are affected by the original issue (CB-14166). As we have seen during the excellent debugging of this issue (thanks @knight9999 and @janpio), tiny differences in your setup can make a big difference in the end result. So it's important to get as much coverage as possible here.
Special thanks to @oliversalzburg for mentioning his handy tool
spodrto me in raphinesse#1. It made building the above test suite way easier. Check it out if you want to easily setup and manage a bunch of linked npm packages (like Cordova tooling) without having to go full-mono-repo as is required by other tools likelerna.Finally, the commit that is prefixed with
TMPis a temporary work-around for a weird issue that I encountered locally (#49). I have no idea what's happening there, but it has absolutely nothing to do with this change. So I took it out of the equation, so please ignore for now.