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

not-so-quick fix sanity check for outside of PRs #172

Merged
merged 14 commits into from Mar 1, 2020
Merged

Conversation

@birm
Copy link
Member

birm commented Feb 28, 2020

This follows #171 and should hopefully fix the oversight for non pr-based tests

In doing so, I've made some other travis changes, and I'm not sure if they're desired.

birm added 8 commits Feb 28, 2020
@birm birm changed the title quickfix sanity check for outside of PRs not-so-quick fix sanity check for outside of PRs Feb 28, 2020
birm added 3 commits Feb 28, 2020
@birm

This comment has been minimized.

Copy link
Member Author

birm commented Feb 28, 2020

If someone other than me merges this, please squash!

Stages didn't work how I expected them to.

Anyway, after ruining my fork's master branch (for now), it looks like the sanity check is indeed not checked on master.
https://travis-ci.org/birm/ensmallen/builds/656138653?utm_medium=notification&utm_source=github_status

I'm now trying to see if I can get it to sanity check after the test, but I may/should sleep before then

@birm birm requested review from zoq and rcurtin and removed request for zoq Feb 28, 2020
HISTORY.md Outdated Show resolved Hide resolved
Copy link
Member

rcurtin left a comment

@birm thanks for the fix! To me, it looks like it's working fine. Let me know if I overlooked something though. 👍

.travis.yml Outdated Show resolved Hide resolved
HISTORY.md Outdated Show resolved Hide resolved
@coatless

This comment has been minimized.

Copy link
Contributor

coatless commented Feb 28, 2020

@birm any reason for the step approach? This seems to infringe on the results of the latest build.

I thought the goal was to preserve existing tests + add the history check. Changing the if statements should be enough. c.f.

#171 (comment)

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Feb 28, 2020

@birm any reason for the step approach? This seems to infringe on the results of the latest build.

I thought the goal was to preserve existing tests + add the history check. Changing the if statements should be enough. c.f.

#171 (comment)

Ah, yeah, maybe that would be easier to go with (I overlooked that comment, sorry). Personally there's no preference from my side, I think it's great to see the support either way. :)

@birm

This comment has been minimized.

Copy link
Member Author

birm commented Feb 28, 2020

The reasons I decided to use stages were mostly about filtering out the sanity task entirely, and making it clearer that it was a separate kind of thing in contrast to the build & ctest job.
My ulterior motive is that I think this may make it easier/clearer to have travis do other such parallel tasks such as this in the future.
That said, I'm not intensely committed to this concept.

@zoq

This comment has been minimized.

Copy link
Member

zoq commented Feb 28, 2020

I think, if we are going to add more tasks in the future it makes sense, I guess we could put: #116 in that other stage as well? But @coatless is right, the fix he proposed would fix the issue as well.

@coatless

This comment has been minimized.

Copy link
Contributor

coatless commented Feb 28, 2020

@birm so, you basically get with filtering:

Screen Shot 2020-02-28 at 4 17 53 PM

Though, I'm confused.... Under this setup, does the ARMADILLO=latest build fail if a HISTORY.md entry isn't present?

@coatless

This comment has been minimized.

Copy link
Contributor

coatless commented Feb 28, 2020

For the record, @birm solution is much better for long-term if using Travis. Mine is a very narrow short-term fix.

@birm

This comment has been minimized.

Copy link
Member Author

birm commented Feb 29, 2020

@coatless No, it won't fail the job, one stage only does the sanity check, so it it just made it easier to filter by having just two jobs. (I had some unexplained problems with three jobs and stages, but I don't think it's about the number). should I try to change it for readability?

@zoq

This comment has been minimized.

Copy link
Member

zoq commented Feb 29, 2020

Personally, I like the split.

@zoq
zoq approved these changes Feb 29, 2020
Copy link
Member

zoq left a comment

Looks good to me.

@birm birm merged commit 29ff827 into mlpack:master Mar 1, 2020
3 of 4 checks passed
3 of 4 checks passed
Memory Checks Build triggered for merge commit.
Details
Static Code Analysis Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zoq

This comment has been minimized.

Copy link
Member

zoq commented Mar 1, 2020

@birm @coatless Looks like we missed something: #174

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.