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

Do not install ccache unless shared_ccache is used in binarydeb #1021

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented Feb 6, 2024

Currently the sharing of compilation artifacts through ccache is conditional after #844, which enable the support for sharing with the host using the shared_ccache option in the build files. The ccache binaries are currently being installed unconditionally thought.

The PR changes the behavior to install the ccache binaries only when the shared_ccache option is being used.

Currently the sharing of compilation artifacts through ccache
is not being done unless the shared_ccache is in use. The ccache
binaries are currently being installed unconditionally.

The PR changes the behaviour to install the ccache only when
the shared_ccache option is being used.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks good. One nit about the import ordering.

@@ -16,7 +16,7 @@
import copy
import sys

from ros_buildfarm.argument import add_argument_append_timestamp
from ros_buildfarm.argument import add_argument_append_timestamp, add_argument_install_ccache
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're doing one per line for all the others. We should do that here too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -16,7 +16,7 @@
import copy
import sys

from ros_buildfarm.argument import add_argument_append_timestamp
from ros_buildfarm.argument import add_argument_append_timestamp, add_argument_install_ccache
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're doing one per line for all the others. We should do that here too. Note that will move it down to keep in alphabetical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm after the fixup

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

Successfully merging this pull request may close these issues.

3 participants