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

Initial implementation #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Initial implementation #1

wants to merge 2 commits into from

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Feb 6, 2023

No description provided.

@cottsay cottsay added the enhancement New feature or request label Feb 6, 2023
@cottsay cottsay self-assigned this Feb 6, 2023
@cottsay cottsay requested a review from sloretz February 6, 2023 17:37
@sloretz
Copy link

sloretz commented Feb 7, 2023

Do I understand correctly that this works by considering the expected name of the build directory (--build-base or --test-result-base or default to "build"), and finding the first ancestor directory containing that folder (or relative path) with a .built_by file inside? Then it changes the current working directory to the ancestor containing the build directory?

A tradeoff with this method is relative paths to other arguments won't work as expected. Say I'm in a workspace I built before at ~/ws. I'm in the ~/ws/src folder and I have two folders full of packages ~/ws/src/foo and ~/ws/src/bar. I only want to build bar, so I pass colcon build --base-paths bar, but colcon doesn't see a folder at ~/ws/bar and so no packages are built.

Another funny relative path is say I'm in ~/ws/src, and I forget I have this extension so I colcon build --build-base ../build. Colcon seems to build the workspace in the current directory.

@cottsay
Copy link
Member Author

cottsay commented Jul 13, 2023

Picking this up again.

Thanks for your thoughts, @sloretz. I think the tradeoffs you mention are rather prohibitive to deploying this as-is.

Do I understand correctly that this works by considering the expected name of the build directory (--build-base or --test-result-base or default to "build"), and finding the first ancestor directory containing that folder (or relative path) with a .built_by file inside? Then it changes the current working directory to the ancestor containing the build directory?

That's accurate. The rest of the colcon routines operate as if invoked from the workspace root directory, which importantly includes the resolution of relative paths.

A tradeoff with this method is relative paths to other arguments won't work as expected...

I beat my head against this for quite a while, and I couldn't arrive at an acceptable solution without making changes to colcon-core. The best I can come up with is this change, which should address this concern specifically: colcon/colcon-core@master...cottsay/resolve-path-args

The same change is necessary in colcon-recursive-crawl, and theoretically any extension which adds arguments which may be relative paths: colcon/colcon-recursive-crawl@master...cottsay/resolve-path-args


I also had to make a change to colcon-alias to handle changes to the working directory: colcon/colcon-alias@main...cottsay/restore-working-directory

With all of these changes combined, I was able to define an alias using: colcon alias buildhere --command build --base-paths .

...which allows me to run colcon buildhere in a subdirectory of a workspace and only build that part of the workspace. You could also do something like colcon alias buildthispackage --command build --base-paths --paths . if you expect to be sitting in the root of a package directory, which might be a little quicker as it bypasses the recursive crawl entirely.

Changing the discovery pipeline might not be the best approach to down-selecting the package(s) you actually want to target. We may want to introduce something like --packages-select-subdirectory-of [PATH ...] to handle this more correctly so things like override checking and dependency validation continue to function.


One explicitly unsupported scenario worth mentioning here is trying to change your build_base in a colcon_defaults.yaml file that sits in the root of your workspace, since colcon doesn't know about the colcon_defaults.yaml until the build_base has already been found. I could add a check for colcon_defaults.yaml as an additional heuristic for locating the workspace root, but I'm uneasy about it. Thoughts would be appreciated.

@nuclearsandwich
Copy link
Contributor

nuclearsandwich commented Sep 8, 2023

I could add a check for colcon_defaults.yaml as an additional heuristic for locating the workspace root, but I'm uneasy about it. Thoughts would be appreciated.

What makes you uneasy about it? If it's because colcon-defaults is not a common extension? I think that's fine as a colcon_defaults.yaml file is a pretty strong heuristic if one is present, I'm not even sure that you would need to make it contingent on colcon-defaults being installed.

@cottsay cottsay force-pushed the cottsay/implementation branch from 7970c2c to ad337f6 Compare September 25, 2023 20:57
* Bionic is in ESM and does not package importlib-metadata
* Buster does not package importlib-metadata
* Stretch has Python 3.5 and hasn't been support for a long time now
* Bookworm is the current "stable" Debian
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants