Skip to content
This repository has been archived by the owner on Aug 16, 2022. It is now read-only.

refactor: move external repositories to src/external #374

Conversation

esteve
Copy link
Contributor

@esteve esteve commented May 27, 2022

Signed-off-by: Esteve Fernandez [email protected]

Description

Move external repositories to src/external

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@esteve esteve requested review from xmfcx and kenji-miyake May 27, 2022 13:11
@esteve esteve mentioned this pull request May 27, 2022
4 tasks
autoware.repos Outdated Show resolved Hide resolved
@xmfcx xmfcx force-pushed the 373-move-external-repositories-to-srcexternal-in-the-repos-file branch from 3805986 to cf56cfe Compare June 2, 2022 17:43
autoware.repos Outdated Show resolved Hide resolved
@esteve
Copy link
Contributor Author

esteve commented Jun 9, 2022

@xmfcx are we not keeping the organization names?

@xmfcx
Copy link
Contributor

xmfcx commented Jun 9, 2022

@xmfcx are we not keeping the organization names?

Replied in autowarefoundation/autoware#374 (comment)

autoware.repos Outdated Show resolved Hide resolved
@xmfcx
Copy link
Contributor

xmfcx commented Jun 9, 2022

So, I've restructured the .repos file with following guidelines in mind:

  • We have top level folders:
    • core
    • launcher
    • param
    • sensor_component # I think we should rename this to sensor_driver
    • sensor_kit # I think we should rename this to sensor_launch and maybe move it under launcher
    • universe
    • vehicle
  • Each top level folder/category is separated by an empty line Not possible
  • Top level folders can have an external folder
    • Repositories outside the autowarefoundation should be put under external.
    • If a repository is forked from the autowarefoundation, it doesn't need to be put under external.

@esteve @kenji-miyake I've also pushed a commit to show how it'd look. It is provisional, can be reverted too. Please give your opinions on these guidelines/structure.

autoware.repos Outdated Show resolved Hide resolved
autoware.repos Outdated Show resolved Hide resolved
@@ -51,15 +51,15 @@ repositories:
url: https://github.com/autowarefoundation/autoware_launch.git
version: main
# sensor_component
sensor_component/sensor_component_description:
sensor_component/external/sensor_component_description:
Copy link
Contributor

@kenji-miyake kenji-miyake Jun 9, 2022

Choose a reason for hiding this comment

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

sensor_component # I think we should rename this to sensor_driver

@xmfcx I think it's okay, but some repositories like this (sensor_component_description) aren't driver.

Copy link
Contributor

@xmfcx xmfcx Jun 10, 2022

Choose a reason for hiding this comment

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

@kenji-miyake Shouldn't sensor_component_description be under the vehicle? Since it is about the vehicle specific sensor models and transformations configurations?

And we can name the rest to be drivers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't sensor_component_description be under the vehicle?

Mm, I feel it's unnatural because it's not completely vehicle-specific. It can be shared between multiple vehicles.

By the way, how about splitting drastic changes in another Issue/PR?
I think it's better to limit the scope of this PR to only vendor -> external.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, how about splitting drastic changes in another Issue/PR? I think it's better to limit the scope of this PR to only vendor -> external.

I agree, I just added them as comments because I didn't want to make those changes under this PR.

type: git
url: https://github.com/tier4/tamagawa_imu_driver.git
version: ros2
sensor_component/velodyne_vls:
sensor_component/external/velodyne_vls:
type: git
url: https://github.com/tier4/velodyne_vls.git
version: tier4/universe
Copy link
Contributor

Choose a reason for hiding this comment

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

sensor_kit # I think we should rename this to sensor_launch and maybe move it under launcher

@xmfcx Mm, I think it's a bit different from sensor_launch, it's the launcher of the sensor combination.
But we can consider renaming and moving to some other sections.

Copy link
Contributor

@xmfcx xmfcx Jun 10, 2022

Choose a reason for hiding this comment

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

@kenji-miyake Nevertheless, it is the launcher of the vehicle sensor drivers, I think we should move it under launcher.

Copy link
Contributor

@kenji-miyake kenji-miyake Jun 10, 2022

Choose a reason for hiding this comment

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

There are two types of launchers. I feel it's better to distinguish them.

  1. Component launchers (called by Autoware launcher)
  1. Autoware (the whole system) launcher (calls component launchers)

@esteve
Copy link
Contributor Author

esteve commented Jun 15, 2022

Since I was the one who opened this PR, I can't approve it, but I'm ok with the changes. @xmfcx I'm assuming you're ok with your own changes 😅 so this should be ready to be merged.

esteve and others added 4 commits June 15, 2022 10:45
Signed-off-by: M. Fatih Cırıt <[email protected]>
Signed-off-by: M. Fatih Cırıt <[email protected]>
Co-authored-by: Kenji Miyake <[email protected]>
@esteve esteve force-pushed the 373-move-external-repositories-to-srcexternal-in-the-repos-file branch from 7b93463 to b1b6c22 Compare June 15, 2022 08:45
@kenji-miyake kenji-miyake enabled auto-merge (squash) June 15, 2022 08:53
@kenji-miyake kenji-miyake merged commit eeaec36 into main Jun 15, 2022
@kenji-miyake kenji-miyake deleted the 373-move-external-repositories-to-srcexternal-in-the-repos-file branch June 15, 2022 08:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants