-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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 AppVeyor CI #1865
Comments
The lerna team mentioned there may be async contention issues on Windows, maybe someone can try setting the |
You set the concurrency for AppVeyor, the flag I was referring to was a flag you need to pass to Lerna itself. 😄 |
Sorry, my bad! Added the concurrency flag to Lerna commands. Will check if this fixes the AppVeyor issue, if yes, will add logic to enable this flag only on AppVeyor to avoid unnecessary slowdowns otherwise. |
Setting concurrency to 1 on Lerna still didn't fix the issue unfortunately |
I believe it may have fixed the erratic failures, but the |
Looks like it's a known problem, AppVeyor updated Git and this regressed, see appveyor/ci#1426. |
Great job nailing it down. |
Unfortunately build is passing just for the |
Can we cherry pick the same fix there? |
I actually cherry-picked the fix from |
Let's keep it open for now then |
Perfect! Any remaining issues are due to lerna bugs which are less present on VS2017 image. Can you keep us posted for when they fix the curl path issue? Thanks! I want to close this since further improvement is unactionable by us (unless if we switch from lerna bootstrap to a lerna exec npm install with concurrency 1 then bootstrap). I just want to make sure the lerna team is aware of this issue. I believe they are. Could you double check for me? I'm tied up for a few days. |
As for the curl path issue, I'll keep an eye on this. As for the Lerna issue, it seems that there are some issues open already but all are mentioning yarn. The failing builds were all using npm, and all failed at the same exact place. Opened issue lerna/lerna#716 on the Lerna repo. It might also just be a memory issue as mentioned in lerna/lerna#338. In the meantime, take a look at this commit darrenscerri@19cda22. There's a script that adds the |
If that consistently fixes the problem it may be worth implementing in a slightly different way. We should handle it in the e2e test files (sh), not as a node script. |
I will run the build multiple times to confirm that it consistently fixes the problem and will report back. Check out the latest commit darrenscerri@19cda22. I moved the script in the Since this is AppVeyor-specific, I don't agree with putting this logic in the e2e EDIT: Still failing :( |
Seems like it’s fine now. |
It’s been broken for a few days.
You can look at CI failures in all recent PRs and try to figure out what happened.
The text was updated successfully, but these errors were encountered: