Skip to content
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

appveyor: skip older pr builds #4591

Closed
wants to merge 3 commits into from
Closed

appveyor: skip older pr builds #4591

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 1, 2017

No description provided.

@ghost
Copy link
Author

ghost commented Jul 1, 2017

request: trivial

@pradyunsg pradyunsg added C: automation Automated checks, CI etc type: maintenance Related to Development and Maintenance Processes skip news Does not need a NEWS file entry (eg: trivial changes) labels Jul 2, 2017
@pradyunsg
Copy link
Member

@xoviat Could you trigger 2 builds on this PR such that one would get cancelled due to these changes?

@ghost
Copy link
Author

ghost commented Jul 2, 2017

@pradyunsg Done.

pradyunsg
pradyunsg previously approved these changes Jul 2, 2017
@pradyunsg
Copy link
Member

@dstufft @pfmoore Could one of you take a look at this?

@pfmoore
Copy link
Member

pfmoore commented Jul 2, 2017

It looks like it does what it says (in other words, the Powershell command added appears to do what the comment suggests it does) but as to whether the idea is good, I don't know.

The Appveyor tests are only the unit tests, so they typically don't take anywhere near as long as the Travis tests (even after recent improvements). So I'm not sure the time saved here is that important. There's 8 appveyor tests, each taking just over a minute, And if I understand the point of the change, it's only likely to make a difference if within that 8 minute window, the author pushes an extra commit to the PR. That seems fairly unlikely, if I'm honest.

I've no objection to improvements like this in principle. But given the shortage of Windows (and specifically Powershell) expertise among the pip developers, I'd be inclined to keep the Appveyor scripts as simple as possible, and hence reject this PR as unnecessary.

@pradyunsg
Copy link
Member

pradyunsg commented Jul 3, 2017

@pfmoore

There's 8 appveyor tests, each taking just over a minute

I am not sure if you saw the AppVeyor builds for #4589? Those held up the build queue for what was a few hours since it caused a regression in pip resulting in a hung tox.

I believe that it was the motivation for making this change - which I too agree to; other PRs did not have their builds run until that build finished, which affected me too.

I'd be inclined to keep the Appveyor scripts as simple as possible, and hence reject this PR as unnecessary.

Sounds fine by me; I don't think we'll get such PRs (that makes changes that hang the build as a result of them) often anyway. That said, if what I said above does change your position and this gets merged, I'm good too.


In short, I'm fine regardless of whether this is merged or not, slightly preferring that this gets merged.

@pfmoore
Copy link
Member

pfmoore commented Jul 3, 2017

I am not sure if you saw the AppVeyor builds for #4589?

No I hadn't. If this PR is motivated by that situation, rather than a simple "let's make the tests faster" motivaton, then I see your point and agree this would be good to have.

I'm still a little concerned about the maintenance - let's see what @dstufft thinks, and if he's OK with it then we can merge it.

@ghost
Copy link
Author

ghost commented Jul 3, 2017

I actually picked this up from obspy; put in by @barsch in obspy/obspy@fedff4b.

@pradyunsg pradyunsg dismissed their stale review July 4, 2017 13:31

I actually just gave this some thought and I think it might be better to actually timeout on AppVeyor if tests run for more than 5 minutes. Making a PR for that.

@ghost ghost closed this Aug 12, 2017
@ghost ghost deleted the patch-1 branch August 12, 2017 21:49
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: automation Automated checks, CI etc skip news Does not need a NEWS file entry (eg: trivial changes) type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants