-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Enable travis ci build. #208
Conversation
scripts/travis.sh
Outdated
@@ -0,0 +1,207 @@ | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need travis.sh instead of just using apollo.sh? Looks like they are largely identical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apollo.sh always source apollo_base.sh which needs user to input "y or Y" to accept the license.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly add an option to apollo.sh to skip sourcing it, otherwise keeping this and apollo.sh in sync might be a pain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is remove "accept license" function from apollo_base.sh. I don't think it is good to put it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@siyangy I fixed your concern, and remove the travis.sh file. Now only the travis ci configure file is needed.
.travis.yml
Outdated
install: skip | ||
script: | ||
- docker exec apollo_dev bash -c "./apollo.sh clean" | ||
- docker exec apollo_dev bash -c "./apollo.sh build" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we run test and lint here by ./apollo.sh check other than build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the first step to enable this ci build feature. We can add more jobs later on by editing the configure file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@siyangy Due to our repo does not have esd_can lib, many tests will fail (49 of 74 fails). I only add "build" and "lint" to the CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should definitely fix this. Unit test should be included in CI. It doesn't have to be fixed in this PR, though.
Btw, next time please add a new commit instead of amending the existing one so that the reviewer would know what change you made and the comments won't be lost. GitHub will squash all the commits into one upon merge.
Resolve issue #82 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed with WangLei
No description provided.