-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
build: fix appveyor cache failures #10281
build: fix appveyor cache failures #10281
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just marking "request changes" so we don't accidentally merge this. do your thing
Cool, thanks @connorjclark ! |
Build https://ci.appveyor.com/project/paulirish/lighthouse/builds/30577378 succeeded without any 7z.exe warning output; this was a result of reverting the |
@brendankenny @connorjclark This took a little while to unravel: it looks like the cause of the That could be due to recursion in the symlink (assuming it behaves similarly to the environment tested in during this PR, it'll symlink from the working directory's Either way, performing an unlink prior to saving the build cache should clear up the problem. The |
This change is hopefully fairly innocuous and may improve AppVeyor's build caching efficiency (plus remove some noise from their build logs). Let me know if I can help add any more context! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the proof is in the appveyor pass to me, thanks very much @jayaddison! 🎉
@connorjclark feel free to just dismiss your change request if you don't care to review but assigning you for now |
thanks for looking into this! just curious, do you work at appveyor or are you just really invested in the health of our CI? :) |
@connorjclark The latter :) I've no connection to AppVeyor (hadn't heard of them until following lighthouse, in fact). Just a little puzzle that I couldn't help but try to solve after I noticed it. |
@connorjclark Not that you really asked, but just to tell a bit more of the story - I've worked in startups/tech for a long time and taking an extended 'time out' at the moment and working on a personal project given the spare time. One of the lifestyle / work changes I'm experimenting with is to throw the pragmatism of startups out of the window. Basically in the past it's generally been a case of 'solve problems well enough to continue on to more important issues' (as long as it's safe & secure to do so, and as long as the maintenance burden isn't prohibitive, etc). Given the new circumstances, I've got a chance to spend as long as desired on any given problem. In the case of A principle I'm following is to fix things upstream, and to not leave any negative externalities before moving on to the next task. As far as I'm concerned I'll be done once this test expectation is restored (I temporarily removed it before a previous axe-core version bump, but then fixed the root cause upstream and now that that's merged, can undo the negative impact). So, strictly speaking this build fix wasn't something I felt compelled to do - but I had a bit of time to look at it while my PR builds were running, and practicing How long this all turns out to be possible for is another question, but it's been enjoyable so far. Edit: fix a link |
Thanks @jayaddison! One of the most enjoyable PR comments I've ever had the pleasure to read :) |
Summary
Noodling around with the appveyor build failures noticed here.
This PR will become a build-related fix, if & when the root cause is discovered and a fix determined.
Related Issues/PRs
While entirely separate to the
lighthouse
project and codebase, a similar7z.exe
non-zero exit code was returned after MoOx/phenomic#1005 was merged (build logs here).