-
Notifications
You must be signed in to change notification settings - Fork 59
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
Speedup testing [6501] #6
Conversation
LuisGP
commented
Oct 2, 2019
- Added parallel build
- Check if Fast-RTPS was already built
…if Fast-RTPS was already built.
commands.add(new String[]{"rm -rf Fast-RTPS", OUTPUT_PATH}); | ||
commands.add(new String[]{"git clone -b " + branch + " https://github.com/eProsima/Fast-RTPS.git", OUTPUT_PATH}); | ||
commands.add(new String[]{"mkdir build", OUTPUT_PATH + "/Fast-RTPS"}); | ||
if (Files.notExists(Paths.get(OUTPUT_PATH + "/Fast-RTPS/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.
What if the path exists but using a branch different from the one we currently want? Would we be using the old one?
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.
Take a look at the else
branch.
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.
Sorry I completely missed that :(
Instead of cloning, why aren't we checking first if Fast-RTPS is installed on the system? Also, if Fast-RTPS is required for testing, IMHO it should be made as a dependency, not cloned on build time. |
Typically, we never install Fast-RTPS system-wide. This way, we can test Fast-RTPS-Gen against a concrete Fast-RTPS branch, which would not be possible if we use an already installed Fast-RTPS. As you said, Fast-RTPS is only required for testing, so keep Fast-RTPS-Gen without such dependency is cleaner, I think. |
commands.add(new String[]{"rm -rf Fast-RTPS", OUTPUT_PATH}); | ||
commands.add(new String[]{"git clone -b " + branch + " https://github.com/eProsima/Fast-RTPS.git", OUTPUT_PATH}); | ||
commands.add(new String[]{"mkdir build", OUTPUT_PATH + "/Fast-RTPS"}); | ||
if (Files.notExists(Paths.get(OUTPUT_PATH + "/Fast-RTPS/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.
Sorry I completely missed that :(