Fix overlays functionality #34
Conversation
|
@kilian-funk-apexai The failing jobs will be fixed when my earlier MR gets merged. I can then rebase this one to allow the jobs to pass. |
revati-naik-apex
force-pushed
the
32-fix-overlays-functionality
branch
2 times, most recently
from
491d5dd to
6f2706f
Compare
|
@kilian-funk-apexai The MR is ready. Also, do you think we should update the documentation to add usage in an external workspace with customer |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
revati-naik-apex
force-pushed
the
32-fix-overlays-functionality
branch
from
5740e5a to
b57a74a
Compare
Yes, please. That would be great. |
revati-naik-apex
force-pushed
the
32-fix-overlays-functionality
branch
2 times, most recently
from
4070d67 to
69eafeb
Compare
README.md
Outdated
| ) | ||
| ``` | ||
|
|
||
| Note: The `repos_index_overlays` field is optional and can be used to pass additonal `*.repos` files containing more ROS2 repositories |
There was a problem hiding this comment.
The reason for the overlays is not to add additional repos but rather a way to declare the BUILD files. That is a syntax extension compared to the original .repos format of the VCS tool used by ROS.
So I would write:
| Note: The `repos_index_overlays` field is optional and can be used to pass additonal `*.repos` files containing more ROS2 repositories | |
| Note: The `repos_index_overlays` field is optional and can be used to pass additonal `*.repos` files declaring BUILD.bazel files to be injected into the ROS2 repositories. |
There was a problem hiding this comment.
@kilian-funk I confused it to be another list of *.repos file. When I looked at the code, I saw that an additional bazel.repos is passed. Also, here it says it needs to be a list of *.repos file. So again, the overlays can only be repositories with BUILD file details? I am still trying to figure out the exact use case here.
Please correct me if I am wrong. This repo uses, by default, the @ros2//:ros2.repos file. We overlay it with the bazel.repos file to pass the BUILD file paths (more like associate) to the corresponding package.
There was a problem hiding this comment.
The format for the .repos files is not our invention. It comes from the open-source VCS tool and does not allow the definition of BUILD files. So the purpose of the overlay files is to amend a .repos file with something that allows the definition of BUILD files. Even if the technical implementation is all the same, the intention is to allow users to reuse a .repos file taken from open source and augment it with additional stuff we need that is not compatible with VCS. The documentation should tell you about the intent, because that is what you cannot read from the code.
There was a problem hiding this comment.
@kilian-funk In that case, is this commit 483ffcf needed? Because what this does is adds the entries from overlays under repositories to the existing repos dict.
There was a problem hiding this comment.
Doesn't that code add the BUILD files to the repositories struct?
There was a problem hiding this comment.
@kilian-funk No, It added new entries of repositories in the overlay files to the main .repos files. This was my previous confusion that this field is used to pass additional repos. Without this code, the overlay file contents (BUILD files) are added to the existing repo data in the main .repos file.
revati-naik-apex
force-pushed
the
32-fix-overlays-functionality
branch
from
69eafeb to
27dc8f9
Compare
revati-naik-apex
force-pushed
the
32-fix-overlays-functionality
branch
from
27dc8f9 to
ad5110e
Compare
|
|
||
| def build_http_archive_load_command(repo, spec): | ||
| return f"""\ | ||
| patches = ',\n '.join(f'"{i}"' for i in spec.get('patches', [])) |
There was a problem hiding this comment.
Why do you change the formatting of the content?
I believe the first list item is not in the right place. It should start in a new line.
repos/config/detail/lock_repos.py
Outdated
| try: | ||
| overlay_content = yaml.safe_load(overlay_file) | ||
| except yaml.YAMLError as exc: | ||
| print(f"Error parsing YAML file {overlay}: {exc}") |
There was a problem hiding this comment.
A parsing error is a severe error. Don't you think the script should error out in that case?
The files passed during the configure stage using the
repos_index_overlaysare not accounted for when generating the setup file.