.travis.yml: make package pulls conditional. #2292

dot-asm · 2017-01-26T00:05:11Z

No description provided.

richsalz

once the build passes, approve.

dot-asm · 2017-02-06T10:01:40Z

Well, it appears that I've figured it out, i.e. it works as latest red cross from travis is unrelated. But after melt-down and make-over travis removed "profiling" information from [raw] log files. I mean earlier there were 'duration' fields recorded for each step and one could see what are the gains, but not anymore :-( I'd also like to argue in favour of configuring with no-makedepend (makedepend makes lesser sense in build-test-and-throw-away) and omitting test_fuzz in mingw tests (it's grotesquely wasteful).

richsalz · 2017-02-06T13:56:42Z

I agree it's not needed to do 'make depend' on a clean CI build. But maybe doing it once finds bugs in make depend? Also agree on the others. So a preliminary plus-one from me on your PR when you do it :)

dot-asm · 2017-02-06T15:54:55Z

There is "fun" dependency between no-makedepend and --strict-warnings. On related note there was report that it fails to recognize gcc if it's command line doesn't have gcc in name. Anyway, rationale behind no-makedepend is that I/O seems to be expensive (for example in attempts to include Android builds quite a time was spent on just unpacking stuff, more time than just downloading it), and makedepend is a bunch of small files...

dot-asm · 2017-02-18T12:39:19Z

exit in before script make job "successful" and it takes less than a minute. Unfortunately one still has to load new compilers, which would be a waste... As mentioned in #2661 idea is to formulate a criteria by which to short-circuit most of the jobs pretending that they were successful. Additional criteria, i.e. in addition to easy-to-forget tag in commit, can be to tell apart merge requests and "official" repo commits.

levitte · 2017-02-18T13:36:12Z

I'm not sure what that exit 0 in the before script is supposed to accomplish. Does it stop that section there?

dot-asm · 2017-02-18T13:59:04Z

what that exit 0 in the before script is supposed to accomplish.

Idea is to opt-in commits for full tests, right? For this you need a way to terminate a job and mark it as "success". And you want to terminate it as early as possible. exit 0 in beginning of before script seems to allow that. So now I have to figure out how to identify opt-in tag and not call exit...

levitte · 2017-02-18T18:18:29Z

.travis.yml

    exclude:
        - os: linux
          compiler: clang
        - os: osx
          compiler: gcc

 before_script:
+    - env
+    - if [ "x$TRAVIS_EVENT_TYPE" == "xpull_request" -a -n "$EXTENDED_TEST" ]; then


Why do people insist on that 'x'? Utterly unnecessary when you have quotes.

I can't answer for people :-) For me I suppose it's kind of better-overdo-than-not... But yes, it can be omitted.

dot-asm · 2017-02-18T18:57:08Z

So the suggestion is to mark specific targets with EXTENTED_TEST=yes in .travis.yml and idea is to execute them only if last commit has [extended tests] tag. It's still quite wasteful in sense that it takes 40 seconds to 1 and half minute to decide if test is to be executed. It might be appropriate to reverse the logic and mark non-extended targets with REQEUIRED_TEST. It make sense to arrange order of execution so that extended and non-extended are grouped together on web page. So that you don't have figure out which one is extended, you'd only need to figure out where non-extended tests end...

dot-asm · 2017-02-18T19:02:02Z

.travis.yml

    exclude:
        - os: linux
          compiler: clang
        - os: osx
          compiler: gcc

 before_script:
+    - env


Just in case for reference. If we choose to take this path, env can be removed after a repo-commit. As you can see [extended tests] are evaluated when TRAVIS_EVENT_TYPE is pull_request. This asks for question what is TRAVIS_EVENT_TYPE when repo-commit is performed. Once we know env can be removed...

dot-asm · 2017-02-19T21:02:04Z

Re-based and squashed.

levitte · 2017-02-20T11:58:41Z

So the suggestion is to mark specific targets with EXTENTED_TEST=yes in .travis.yml and idea is to execute them only if last commit has [extended tests] tag. It's still quite wasteful in sense that it takes 40 seconds to 1 and half minute to decide if test is to be executed. It might be appropriate to reverse the logic and mark non-extended targets with REQEUIRED_TEST. It make sense to arrange order of execution so that extended and non-extended are grouped together on web page. So that you don't have figure out which one is extended, you'd only need to figure out where non-extended tests end...

I'm not sure I follow... What's the intended practical difference between EXTENDED_TEST and REQUIRED_TEST? Also, the .travis.yml in this PR doesn't show much how you intend this to work, all it does is exit early from before_script, but that doesn't show how you intend tests to be affected by this exercise... Yes, I know, it's still in WIP, but thing is that as long as there's just that early exit, the intention remains unclear (at least in my mind) and it becomes an academic discussion...

dot-asm · 2017-02-20T12:28:04Z

What's the intended practical difference between EXTENDED_TEST and REQUIRED_TEST?

None. It's just a preference question. Do we want to mark "extended" tests or do we want to mark non-extended ones in .travis.yml. I.e. goal is the same, to limit execution of same group of tests. And we can say that definition of the said group by tagging tests with EXTENDED_TEST sucks, and decide to describe the complementary group instead by tagging non-extended tests with REQUIRED_TEST. That's all.

all it does is exit early from before_script,

And that is all one can do(*). I wish it could be solved by simply not running selected subset. In dreamland commits not tagged with [extended tests] would be presented with shortened list of jobs at https://travis-ci.org/openssl/openssl/builds/203241005. But it doesn't work like that, and there is nothing we can do. Early exit is simply the way that is available to us. Once again, idea is simply to abstain from running most expensive tests by default, because it's wasteful in most cases. And wasteful not only from travis-ci resource utilization viewpoint, but it's waste of our time waiting for the outcome. By latter I mean that one of points of having ci is that you wait for its results to evaluate them, isn't it?

(*) Well, of course one can solve the problem by arranging different branches to be watched by travis. Main trunk would be with minimalistic .travis.yml, then there could be branch with extended tests. Question here is who would keep it up-to-date? What would users have to remember to do? I understand that it's probably not as complicated. But so is [skip ci], not complicated that is, but how many times people used it say with just documentation update? In other words I'd say that opt-in directly for master branch should be preferred option...

levitte · 2017-02-20T13:01:44Z

Errrr... I think there's a misunderstanding here, we seem to be talking about very different things. My concern is that if this line does exit 0, it breaks off too early, leaving you with an unconfigured build tree and will make the build fail. Can't you simply set up a variable with content depending on if [extended test] was found or not, and then use that variable where applicable?

dot-asm · 2017-02-20T13:23:31Z

... misunderstanding ... exit 0

exit 0 there is supposed terminate not just before_script, but whole job. And in such way that it's marked green. It should never come to configuration, let alone make... At least that's what preliminary tests have shown... Now it seems to not work anymore... Point was that jobs tagged with EXTENDED_TEST should take like 40 seconds to "complete"... Looking...

levitte · 2017-02-20T13:27:39Z

It doesn't happen because you do a git log -2, and one of the commit messages you get contains [extended test]...

dot-asm · 2017-02-20T13:33:42Z

Duh... Thanks!

dot-asm · 2017-02-20T13:46:32Z

So I've amended last commit replacing [extended tests] with ~extended tests~ and as you can see already now extended tests come out green. This also shows that if you were on the other side you can opt-in by simply amending last commit. Yes, non-extended test results would be wasted, i.e. executed twice, but overall it should be saving... Well, it's still rather wasteful, because just to come to point of decision you update and install packages and it's at least 40 seconds. Do keep in mind that it's not alternative to #2661, rather complement...

FdaSilvaYY · 2017-02-20T22:27:35Z

Suggestion : git log $TRAVIS_COMMIT_RANGE
See https://docs.travis-ci.com/user/environment-variables/#Default-Environment-Variables

dot-asm · 2017-02-20T22:44:50Z

Suggestion : git log $TRAVIS_COMMIT_RANGE

Cool! Thanks! I'll be having a look at ordering builds on page so that "extended" and "minimal" are not intermixed, and will incorporate the suggestion. But not today. Thanks again.

FdaSilvaYY · 2017-02-20T23:55:04Z

I get an idea, while following this PR ; please check #2692...

dot-asm · 2017-02-21T10:30:23Z

Suggestion : git log $TRAVIS_COMMIT_RANGE

After additional consideration I'm leaning toward git log -1 $TRAVIS_COMMIT_RANGE, i.e. equivalent of gilt log -2, just more accurate. Rationale is that if you address an issue as part of review, it's not given that it classifies for expensive tests, even if request initially did. I.e. rationale is that you would still have to make judgment and explicitly opt-in even follow-up commits.

Since CI is engaged on per merge request basis, it can be wasteful to run each request through all the tests, especially those resource consuming. Idea is to mark most of tests as "extended" and provide a way to opt-in by marking last commit with ~extended tests~ tag. It's still not as optimal as one could wish, as decision to skip a test still requires machine time, and is taken in configured environment, i.e. with updates and additional packages installed.

[to be squashed]

dot-asm · 2017-02-24T08:32:30Z

This is finalized. Besides conditional package install, it limits test coverage unless last commit is tagged with [extended tests].

levitte · 2017-02-24T12:11:22Z

So, uhmmmm, if I understand correctly, the purpose of [extended test] does not just cover testing (as in running make test), but everything including building in the first place. Was that the intention? That's the effect as currently implemented.

levitte · 2017-02-24T12:15:17Z

Also, you mentioned somewhere else (email?) that the cross compiled builds could do without test_fuzz. I totally agree with that idea, +1 for adding a TESTS="-test_fuzz" on those.

dot-asm · 2017-02-24T12:45:17Z

purpose of [extended test] does not just cover testing (as in running make test), but everything including building in the first place. Was that the intention?

Yes. Even though you are correct implying that lack of [extended test] tag could translate to build-only thing, it makes lesser sense to compile a number of targets, sanitizers of course, without actually executing the outcome. Second counter-argument is to define bare-minimum amount of tests, as for majority of pull requests just few builds really suffice, and doing more would probably be overkill when it comes to resource consumption, i.e. it's about being cooperative. [I do wish there was a way to cancel part of jobs without them being started, or earlier than it's done now, but in a way that doesn't result in red cross]. Third counter-argument is that since you can "request" extended testing afterwards, by amending last commit and adding the tag, skipping jobs in their entirety would result in lesser waste. [I mean requesting extending testing afterwards would effectively waste resources taken for bare-minimum and skipped jobs in the initial request. If lack of [extended test] tag translated to build-only, the waste would be higher.]

dot-asm · 2017-02-24T12:48:12Z

cross compiled builds could do without test_fuzz

It's done implicitly in if [ -n "$CROSS_COMPILE" ]; then clause.

levitte · 2017-02-24T13:09:15Z

Yes. Even though you are correct implying that lack of [extended test] tag could translate to build-only thing, ...

Noted, and I don't disagree with that, just wanted to be clear what we mean with "test" in this context.

cross compiled builds could do without test_fuzz

It done implicitly in if [ -n "$CROSS_COMPILE" ]; then clause.

Ah. Actually, I would prefer a more explicit approach, i.e. requiring it be explicitely declared in the matrix. More explicit == less magic == goodness, in my mind.

[to be squashed][skip ci]

dot-asm · 2017-02-24T13:35:24Z

cross compiled builds could do without test_fuzz

It done implicitly in if [ -n "$CROSS_COMPILE" ]; then clause.

Ah. Actually, I would prefer a more explicit approach, i.e. requiring it be explicitely declared in the matrix. More explicit == less magic == goodness, in my mind.

Pushed.

levitte · 2017-02-24T13:55:10Z

.travis.yml

@@ -117,7 +174,7 @@ script:
    - if [ -z "$BUILDONLY" ]; then
          if [ -n "$CROSS_COMPILE" ]; then
              sudo apt-get -yq --no-install-suggests --no-install-recommends --force-yes install wine;
-              export EXE_SHELL="wine" WINEPREFIX=`pwd`;
+              export EXE_SHELL="wine" WINEPREFIX=`pwd`


Errr, please put the ; back

([skip ci] is inconvenient at times, eh? ;-) )

Oops. Fixed. Thanks.

levitte

I'm happy with this

Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #2292)

Since CI is engaged on per merge request basis, it can be wasteful to run each request through all the tests, especially those resource consuming. Idea is to mark most of tests as "extended" and provide a way to opt-in by marking last commit with [extended tests] tag. It's still not as optimal as one could wish, as decision to skip a test still requires machine time, and it's taken in configured environment, i.e. with updates and additional packages installed... Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #2292)

richsalz approved these changes Jan 26, 2017

View reviewed changes

dot-asm force-pushed the travis-conditional-install branch from 723299d to b679262 Compare February 6, 2017 11:11

richsalz added the reviewed label Feb 16, 2017

dot-asm mentioned this pull request Feb 17, 2017

Reduce Travis impact #2661

Closed

dot-asm force-pushed the travis-conditional-install branch from b679262 to 15de793 Compare February 18, 2017 09:52

levitte reviewed Feb 18, 2017

View reviewed changes

dot-asm commented Feb 18, 2017

View reviewed changes

FdaSilvaYY mentioned this pull request Feb 19, 2017

.travis.yml: remove osx from build matrix. #2681

Closed

dot-asm force-pushed the travis-conditional-install branch from 00f18b5 to 2337a85 Compare February 19, 2017 20:56

dot-asm force-pushed the travis-conditional-install branch from 2337a85 to 5974064 Compare February 20, 2017 13:37

.travis.yml: make package pulls conditional.

99769be

Andy Polyakov added 2 commits February 23, 2017 19:49

.travis.yml: reorginize order [to be squashed]

f50bbe4

dot-asm force-pushed the travis-conditional-install branch from 5974064 to f50bbe4 Compare February 23, 2017 18:49

.travis.yml: run test_fuzz only in extended tests, minor tweaks.

0352a54

[to be squashed]

dot-asm changed the title ~~WIP: .travis.yml: make package pulls conditional.~~ .travis.yml: make package pulls conditional. Feb 24, 2017

.travis.yml: make -test_fuzz explicit in wine-based tests.

ccee65e

[to be squashed][skip ci]

levitte requested changes Feb 24, 2017

View reviewed changes

.travis.yml: fix typo [to be squashed].

cbeb941

levitte approved these changes Feb 24, 2017

View reviewed changes

levitte pushed a commit that referenced this pull request Feb 24, 2017

.travis.yml: make package pulls conditional.

31c1db9

Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #2292)

dot-asm closed this Feb 24, 2017

dot-asm mentioned this pull request Feb 24, 2017

.travis.yml: make package pulls conditional. #2735

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.travis.yml: make package pulls conditional. #2292

.travis.yml: make package pulls conditional. #2292

dot-asm commented Jan 26, 2017

richsalz left a comment

dot-asm commented Feb 6, 2017

richsalz commented Feb 6, 2017

dot-asm commented Feb 6, 2017

dot-asm commented Feb 18, 2017

levitte commented Feb 18, 2017

dot-asm commented Feb 18, 2017

levitte Feb 18, 2017

dot-asm Feb 18, 2017

dot-asm commented Feb 18, 2017

dot-asm Feb 18, 2017 •

edited

dot-asm commented Feb 19, 2017

levitte commented Feb 20, 2017

dot-asm commented Feb 20, 2017 •

edited

levitte commented Feb 20, 2017

dot-asm commented Feb 20, 2017 •

edited

levitte commented Feb 20, 2017

dot-asm commented Feb 20, 2017

dot-asm commented Feb 20, 2017 •

edited

FdaSilvaYY commented Feb 20, 2017 •

edited

dot-asm commented Feb 20, 2017

FdaSilvaYY commented Feb 20, 2017

dot-asm commented Feb 21, 2017

dot-asm commented Feb 24, 2017

levitte commented Feb 24, 2017

levitte commented Feb 24, 2017

dot-asm commented Feb 24, 2017

dot-asm commented Feb 24, 2017 •

edited

levitte commented Feb 24, 2017

dot-asm commented Feb 24, 2017

levitte Feb 24, 2017

levitte Feb 24, 2017

dot-asm Feb 24, 2017

levitte left a comment

.travis.yml: make package pulls conditional. #2292

.travis.yml: make package pulls conditional. #2292

Conversation

dot-asm commented Jan 26, 2017

richsalz left a comment

Choose a reason for hiding this comment

dot-asm commented Feb 6, 2017

richsalz commented Feb 6, 2017

dot-asm commented Feb 6, 2017

dot-asm commented Feb 18, 2017

levitte commented Feb 18, 2017

dot-asm commented Feb 18, 2017

levitte Feb 18, 2017

Choose a reason for hiding this comment

dot-asm Feb 18, 2017

Choose a reason for hiding this comment

dot-asm commented Feb 18, 2017

dot-asm Feb 18, 2017 • edited

Choose a reason for hiding this comment

dot-asm commented Feb 19, 2017

levitte commented Feb 20, 2017

dot-asm commented Feb 20, 2017 • edited

levitte commented Feb 20, 2017

dot-asm commented Feb 20, 2017 • edited

levitte commented Feb 20, 2017

dot-asm commented Feb 20, 2017

dot-asm commented Feb 20, 2017 • edited

FdaSilvaYY commented Feb 20, 2017 • edited

dot-asm commented Feb 20, 2017

FdaSilvaYY commented Feb 20, 2017

dot-asm commented Feb 21, 2017

dot-asm commented Feb 24, 2017

levitte commented Feb 24, 2017

levitte commented Feb 24, 2017

dot-asm commented Feb 24, 2017

dot-asm commented Feb 24, 2017 • edited

levitte commented Feb 24, 2017

dot-asm commented Feb 24, 2017

levitte Feb 24, 2017

Choose a reason for hiding this comment

levitte Feb 24, 2017

Choose a reason for hiding this comment

dot-asm Feb 24, 2017

Choose a reason for hiding this comment

levitte left a comment

Choose a reason for hiding this comment

dot-asm Feb 18, 2017 •

edited

dot-asm commented Feb 20, 2017 •

edited

dot-asm commented Feb 20, 2017 •

edited

dot-asm commented Feb 20, 2017 •

edited

FdaSilvaYY commented Feb 20, 2017 •

edited

dot-asm commented Feb 24, 2017 •

edited