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

Update Github Actions template #752

Merged

Conversation

mathias-luedtke
Copy link
Member

@mathias-luedtke mathias-luedtke commented Oct 8, 2021

@mathias-luedtke
Copy link
Member Author

@gavanderhoorn: Please have a look

@mathias-luedtke
Copy link
Member Author

mathias-luedtke commented Oct 8, 2021

@mathias-luedtke
Copy link
Member Author

@gavanderhoorn: I you don't have any objection, I will merge this. It is definitely better than the current version.

Comment on lines 35 to 38
# we always want the ccache cache to be persisted, as we cannot easily
# determine whether dependencies have changed, and ccache will manage
# updating the cache for us. Adding 'run_id' to the key will force an
# upload at the end of the job.
Copy link
Member

Choose a reason for hiding this comment

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

This is a rather specific comment.

Maybe generalise a bit and just state that we don't use any hashing here to detect whether anything has changed, as ccache itself is capable of detecting whether a cache can be reused. Trying to duplicate that logic here in a GHA config seems like a strange thing to want to do.

For anything not a ccache cache, more complex setups (such as the MoveIt one) would be fully warranted. They're just not part of this simple template.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I will rephrase it.

Copy link
Member

@gavanderhoorn gavanderhoorn Oct 15, 2021

Choose a reason for hiding this comment

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

Of course if the ccache is potentially very large it might make sense to try and optimise it a little (ie: avoid downloading it when it's not needed).

But again: the template shows the 'naive' version, just to get people going.

@mathias-luedtke mathias-luedtke merged commit 2b549f7 into ros-industrial:master Nov 11, 2021
@mathias-luedtke mathias-luedtke deleted the fix/action-template branch November 11, 2021 21:22
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.

Update Github Action template
2 participants