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

Avoid running Makefile.main ::build ::tests on web #631

Closed
wants to merge 1 commit into from
Closed

Avoid running Makefile.main ::build ::tests on web #631

wants to merge 1 commit into from

Conversation

dpordomingo
Copy link
Contributor

@dpordomingo dpordomingo commented Apr 16, 2019 •
edited
Loading

blocks #626

The purpose of Makefile.web is to test and build Lookout web, but it was also testing the whole Lookout because Makefile.web includes Makefile, and that one includes src-d/ci/Makefile.main

This commit overrides Makefile.main::test and Makefile.main::dependencies targets to avoid calling them.

  • make -f Makefile.web test will test only the web
  • make -f Makefile.web dependencies will still install $(DEPENDENCIES), but it won't go get all lookoutd dependencies, because they're already handled by make godep

examples of wrong behavior:

Why is this needed?

  • Because otherwise, failures in lookoutd can cause Web Tests Travis stage to fail, what is incorrect.
  • This should also speed up the Web Tests Travis stage to be ~1'3mins less than before.

@dpordomingo dpordomingo added the bug Something isn't working label Apr 16, 2019
@dpordomingo dpordomingo self-assigned this Apr 16, 2019
@dpordomingo dpordomingo force-pushed the fix-makefile-web branch 2 times, most recently from 7840930 to 0e8ea89 Compare April 16, 2019 07:35
@dpordomingo dpordomingo mentioned this pull request Apr 16, 2019
Open
@carlosms
Copy link
Contributor

I don't get how this is supposed to be used now.
make -f Makefile.web build does not build a lookoutd bin.
make build does not include web assets in the web sub command.

Am I missing some new step?

@dpordomingo
Avoid running Makefile.main ::dependencies & ::tests on web
c46eddf 
The purpose of Makefile.web is to test and build
Lookout web, but it was also testing the whole Lookout
because Makefile.web includes Makefile, and that one includes
src-d/ci/Makefile.main

This commit overrides Makefile.main::test and
Makefile.main::dependencies targets to avoid calling them
when using Makefile.web

Signed-off-by: David Pordomingo <[email protected]>
@dpordomingo
Copy link
Contributor Author

You were right; I didn't see it in .travis so I assumed it was not used, but it was a wrong assumption.
I restored the build target.

@dpordomingo
Copy link
Contributor Author

This PR was opened because I find quite disturbing to see Web Tests Travis stage failing because of a failing test outside of frontend dir, even when that's also tested in other Travis stages.

Anyhow, I also realized that Makefile.web could be used instead of Makefile to test everything at once (frontend and non-frontend stuff), so I'll close this PR assuming it was done on purpose.

If you also find this behavior disturbing, we can discuss it in a separate issue, or just reopen/merge this PR.

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

@carlosms carlosms Awaiting requested review from carlosms

Assignees

@dpordomingo dpordomingo

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dpordomingo @carlosms