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.

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

Extend Travis build time-out #11319

Closed
wants to merge 2 commits into from
Closed

Extend Travis build time-out #11319

wants to merge 2 commits into from

Conversation

richsalz
Copy link
Contributor

Replaces #11313 which I could not re-open because I force-pushed before reopening. Who knew? :)

@richsalz richsalz mentioned this pull request Mar 12, 2020
Closed
@levitte
Copy link
Member

levitte commented Mar 12, 2020

From reading this, it seems like travis_wait defaults to a 20 minute timeout, but you can specify something longer:

https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received

@richsalz
Copy link
Contributor Author

Yes, 20 minutes is the default. Let's see what happens because right now it cuts off at 10.

@levitte
Copy link
Member

levitte commented Mar 12, 2020

... you mean at 30

It looks promising!

@richsalz
Copy link
Contributor Author

almost there. updating to 40.

@richsalz
Copy link
Contributor Author

trying again with timeout set to (!!!) 60.

@mattcaswell mattcaswell mentioned this pull request Mar 12, 2020
Closed
2 tasks
@mattcaswell
Copy link
Member

In spite of the commit message saying "Update timeout to 60", the timeout still seems to be 40! Although this time it passed.

@slontis
Copy link
Member

slontis commented Mar 14, 2020

Might try this again just for fun...

@slontis slontis closed this Mar 14, 2020
@slontis slontis reopened this Mar 14, 2020
@richsalz
Copy link
Contributor Author

forgot to add the file to the commit. timeout now set at 60 and re-pushed.

@richsalz
Copy link
Contributor Author

I was going to try making the timeout be a shell variable, and then we could leave it at the default for all but the one problematic platform, but I don't think it's necessary. If every build takes its maximum of an hour, we'll notice the backlog I think. So taking this out of WIP. Should be backported, too.

@richsalz richsalz changed the title WIP: Extend some travis time-outs Extend Travis build time-out Mar 14, 2020
@slontis
Copy link
Member

slontis commented Mar 14, 2020

Well that didn't quite work out as planned - 2 travis timeouts. (It still seemed to timeout after 50 minutes?)

@beldmit
Copy link
Member

beldmit commented Mar 15, 2020

I'm not sure that just increasing timeout is a decent way to deal with this problem...

@levitte
Copy link
Member

levitte commented Mar 15, 2020

Hmmm, the timeouts we see seems to be largely due to make test causing a rebuild. This is at least the case for job #33142.4, and I suspect (it's not as clear) that the same goes for job #33142.5.

This may mean that some dependencies get a later timestamp than expected, causing another recompile. This is actually an old problem that isn't unique to us. I recall that some GNU documentation recommend make && make for exactly this sort of reason when you use the automatic Makefile generation available in gcc and derivatives.

@mspncp
Copy link
Contributor

mspncp commented Mar 15, 2020 •
edited

I encountered this behaviour locally too several times. At some time I suspected that it has to do with the conversion of some *.pod files to *.pod.in files and that it occurs only when I build in parallel (-j 8). But I never got around to investigate that systematically.

@levitte
Copy link
Member

levitte commented Mar 15, 2020

When you fiddle around with branches, it's mostly any changes in header file dependencies that may trigger an extra rebuild.

@richsalz
Copy link
Contributor Author

Hmmm, the timeouts we see seems to be largely due to make test causing a rebuild.

Since we did just build, then perhaps using _tests as the target makes sense. Trying that now.

@levitte
Copy link
Member

levitte commented Mar 15, 2020

Since we did just build, then perhaps using _tests as the target makes sense. Trying that now.

I hope that it works out at all times and doesn't have us depend on luck...

@richsalz
Copy link
Contributor Author

I don't see what could go wrong (famous last words), but it worked. So, ready for review.

@levitte
Copy link
Member

levitte commented Mar 15, 2020

I don't see what could go wrong (famous last words), but it worked. So, ready for review.

Considering the caches are probably PR specific, it's highly probably nothing goes wrong. I've seen things going wrong, i.e. needing a second recompile, but that's been when I've jumped wildly between branches, so..

@levitte levitte added approval: review pending This pull request needs review by a committer branch: master Merge to master branch labels Mar 15, 2020
@levitte
Copy link
Member

levitte commented Mar 15, 2020

I supect that things are different enough between 1.1.1 and master that 1.1.1 might need a separate PR

@richsalz richsalz mentioned this pull request Mar 15, 2020
Closed
@richsalz
Copy link
Contributor Author

1.1.1 PR in #11331

@richsalz
Copy link
Contributor Author

Since this PR seems to address the near-constant CI timeouts, it seems helpful to get this merged sooner rather than waiting?

@levitte
Copy link
Member

levitte commented Mar 17, 2020

Yup.
Second reviewer, please, @openssl

t8m
t8m approved these changes Mar 17, 2020
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Mar 17, 2020
@levitte
Copy link
Member

levitte commented Mar 17, 2020

@t8m, I propose "urgent". Do you agree?

mspncp
mspncp approved these changes Mar 17, 2020
View reviewed changes
Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's urgent.

@mspncp mspncp added approval: ready to merge The 24 hour grace period has passed, ready to merge severity: urgent Fixes an urgent issue (exempt from 24h grace period) and removed approval: done This pull request has the required number of approvals labels Mar 17, 2020
@mspncp
Copy link
Contributor

mspncp commented Mar 17, 2020

I squashed the two commits and slightly edited the joined commit message, see d18dae018278ed4883b6b2f795e315e9d89c7cd9. @richsalz @levitte @t8m do I have your approval to push it like this?

Fix travis timeouts

- Add travis_wait to the build command
- And travis_retry to some apt-get commands.
- Use `make _tests` instead of `make test`

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from https://github.com/openssl/openssl/pull/11319)

@mspncp
Copy link
Contributor

mspncp commented Mar 17, 2020

Oh, sorry, I should have used your pull request title as subject line. Hang on for a second...

Extend Travis build time-out

- Add travis_wait to the build command
- And travis_retry to some apt-get commands.
- Use `make _tests` instead of `make test`

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from https://github.com/openssl/openssl/pull/11319)

mspncp pushed a commit to mspncp/openssl that referenced this pull request Mar 17, 2020
@richsalz @mspncp
Extend Travis build time-out
fd32f91 
- Add travis_wait to the build command
- And travis_retry to some apt-get commands.
- Use `make _tests` instead of `make test`

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from openssl#11319)
@mspncp
Copy link
Contributor

mspncp commented Mar 17, 2020

Done, see fd32f91.

@richsalz
Copy link
Contributor Author

fine with me.

@mspncp
Copy link
Contributor

mspncp commented Mar 17, 2020

Ok, then I'll push it now.

openssl-machine pushed a commit that referenced this pull request Mar 17, 2020
@richsalz @mspncp
Extend Travis build time-out
cde63b7 
- Add travis_wait to the build command
- And travis_retry to some apt-get commands.
- Use `make _tests` instead of `make test`

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from #11319)
@mspncp
Copy link
Contributor

mspncp commented Mar 17, 2020

Merged as cde63b7. Henceforth, thou shalt be called the Mighty Travis Timeout Slayer, @richsalz!

@mspncp mspncp closed this Mar 17, 2020
@richsalz
Copy link
Contributor Author

now you've jinxed it :)

@richsalz richsalz deleted the travis-work-arounds branch March 17, 2020 22:55
@mspncp
Copy link
Contributor

mspncp commented Mar 17, 2020

now you've jinxed it :)

Are you saying I should have kept my mouth shut until Travis has completed? I hope, I didn't mess it up...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte approved these changes

@t8m t8m t8m approved these changes

@mspncp mspncp mspncp approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: urgent Fixes an urgent issue (exempt from 24h grace period)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@richsalz @levitte @mattcaswell @slontis @beldmit @mspncp @t8m