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

[foxy backport]: filesystem helpers: adding remove_all to remove non-empty directories… #80

Merged
merged 2 commits into from Jul 14, 2020
Merged

[foxy backport]: filesystem helpers: adding remove_all to remove non-empty directories… #80

merged 2 commits into from Jul 14, 2020

Conversation

Karsten1987
Copy link
Contributor

foxy backport of #79

@Karsten1987
filesystem helpers: adding remove_all to remove non-empty directories (…
c695e23 
…#79)

* remove_all

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* remove unused variable

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* address review comments

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* apply suggestions

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987 Karsten1987 requested a review from a team as a code owner July 1, 2020 22:56
@Karsten1987 Karsten1987 self-assigned this Jul 1, 2020
@Karsten1987 Karsten1987 added this to Proposed in Foxy Patch Release 1 via automation Jul 1, 2020
@Karsten1987 Karsten1987 moved this from Proposed to In progress in Foxy Patch Release 1 Jul 1, 2020
@allenh1 @Karsten1987
Add cstring include for strcmp (#81)
2f7aaf5 
Signed-off-by: Hunter L. Allen <hunter.allen@ghostrobotics.io>
@ivanpauno
Copy link
Member

ivanpauno commented Jul 2, 2020 •
edited

should features be backported?
Except this is blocking another backport, I don't see much value on adding this to Foxy.

@emersonknapp
Copy link
Contributor

I am also confused about backporting features into LTS, I thought it was a no-no. Maybe it's just that we have to maintain API and ABI compatibility - if so how to we verify that that is the case?

@Karsten1987
Copy link
Contributor Author

my thinking here was given that it's only additions, it's probably fine to backport. I was planning on using it for backports on rosbag2. But I am okay closing this PR if it's the general consensus.

@clalancette
Copy link
Contributor

I am also confused about backporting features into LTS, I thought it was a no-no. Maybe it's just that we have to maintain API and ABI compatibility - if so how to we verify that that is the case?

Right, API and ABI compatibility are absolutely essential. Beyond that, whether to backport a feature to a stable release comes down to how broadly applicable the feature is and the maintainers discretion.

Checking for API compatibility is easy; just look at the diff and see if any of the public APIs have been changed. Checking for ABI compatibility can be a bit more tricky. Basically anything that changes the size of an object is going to break ABI. This usually includes adding virtual methods, adding member variables, and the like. There are some tools around this; one of them is called abi-compliance-checker, and can be installed on Ubuntu with sudo apt-get install abi-compliance-checker. The buildfarm can also be configured to check ABI, by setting test_abi: true, like the rcl entry.

@emersonknapp
Copy link
Contributor

Well, this doesn't touch any classes/objects or existing APIs. So, as just an addition of a new function it seems it's API and ABI compatible, so I have no problem with backporting

@jacobperron jacobperron removed this from In progress in Foxy Patch Release 1 Jul 10, 2020
@jacobperron jacobperron added this to Proposed in Foxy Patch Release 2 via automation Jul 10, 2020
@jacobperron jacobperron moved this from Proposed to In progress in Foxy Patch Release 2 Jul 10, 2020
ivanpauno
ivanpauno approved these changes Jul 13, 2020
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM, considering that this backport is supposed to make easier a fix in rosbag I think it is worth doing 👍.

my thinking here was given that it's only additions, it's probably fine to backport. I was planning on using it for backports on rosbag2. But I am okay closing this PR if it's the general consensus.

@emersonknapp
Copy link
Contributor

emersonknapp commented Jul 13, 2020 •
edited

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp emersonknapp merged commit 61a5c26 into foxy Jul 14, 2020
@emersonknapp emersonknapp deleted the backport_79 branch July 14, 2020 05:56
@jacobperron jacobperron moved this from In progress to Needs release in Foxy Patch Release 2 Jul 20, 2020
@jacobperron jacobperron moved this from Needs release to Released in Foxy Patch Release 2 Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersonknapp emersonknapp emersonknapp approved these changes

@ivanpauno ivanpauno ivanpauno approved these changes

Assignees

@Karsten1987 Karsten1987

Labels
None yet
Projects
No open projects
Foxy Patch Release 2
  
Released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Karsten1987 @ivanpauno @emersonknapp @clalancette @allenh1