add check root dir #383

claireyywang · 2020-09-22T22:44:10Z

Adds a .root_dir marker file when first building a workspace. Check for .root_dir file whenever build or test verbs are invoked, terminate command if marker file not found. closes #369

What this PR does not cover:

discovering marker file in child directories. If colcon build is invoked in ~/, no error/warning will be output by this function, and a marker file will be generated in ~/
if a child directory is wrongly marked as root, user has to manually remove the .colcon file for this warning function to work properly

To test this function, follow build from source instruction here https://colcon.readthedocs.io/en/released/developer/bootstrap.html, then run colcon build in different directories to see the error msg.

Terminal output if colcon build in child dir:

$ colcon build 
[0.620s] ERROR:colcon:colcon build: '/home/claire/colcon-from-source/src/colcon-core' is not marked as the root directory. Please go to '/home/claire/colcon-from-source' for `colcon build`. If the current directory is intended to be the root workspace, please remove the '.root_dir' file in '/home/claire/colcon-from-source'.

still need to add test cases...

Signed-off-by: claireyywang <[email protected]>

codecov · 2020-09-22T23:11:28Z

Codecov Report

Merging #383 into master will decrease coverage by 1.44%.
The diff coverage is 11.11%.

@@            Coverage Diff             @@
##           master     #383      +/-   ##
==========================================
- Coverage   80.01%   78.57%   -1.45%     
==========================================
  Files          56       56              
  Lines        3272     3290      +18     
  Branches      543      546       +3     
==========================================
- Hits         2618     2585      -33     
- Misses        615      654      +39     
- Partials       39       51      +12

Impacted Files	Coverage Δ
colcon_core/verb/build.py	`0.00% <0.00%> (ø)`
colcon_core/verb/__init__.py	`82.66% <7.14%> (-17.34%)`	⬇️
colcon_core/verb/test.py	`28.57% <50.00%> (+0.41%)`	⬆️
colcon_core/shell/bat.py	`71.42% <0.00%> (-19.05%)`	⬇️
colcon_core/environment/path.py	`93.10% <0.00%> (-6.90%)`	⬇️
colcon_core/shell/sh.py	`84.12% <0.00%> (-6.35%)`	⬇️
colcon_core/event/command.py	`94.33% <0.00%> (-5.67%)`	⬇️
colcon_core/subprocess.py	`60.15% <0.00%> (-5.47%)`	⬇️
colcon_core/package_discovery/path.py	`96.07% <0.00%> (-3.93%)`	⬇️
colcon_core/logging.py	`91.37% <0.00%> (-3.45%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update caee5e6...1414381. Read the comment docs.

dirk-thomas

The docblock / messages could be a bit clearer / more precise (and contain spelling mistakes) but let's first focus on the functional parts.

Why is the check limited to the build and test verbs? (Also the whole ticket description doesn't mention test.)

colcon_core/verb/__init__.py

dirk-thomas · 2020-09-22T23:32:57Z

colcon_core/verb/__init__.py

+    :param str this_build_tool: The name of this build tool
+    :raises RuntimeError: if marker file is found in parent directory
+    """
+    original_path = Path(os.getcwd())


Wouldn't it be clearer (easier to test, and more reusable) if the function would have the start path as an argument rather then getting it internally from global state?

colcon_core/verb/__init__.py

Signed-off-by: claireyywang <[email protected]>

claireyywang · 2020-09-30T00:31:05Z

@dirk-thomas Thanks for the timely review. I tried to address most of the comments, but not sure about a few.

Why is the check limited to the build and test verbs?

Those are the only two verbs in colcon-core, and seem to address the feature request in #369. Is there anywhere else that could also use this check?

Wouldn't it be clearer (easier to test, and more reusable) if the function would have the start path as an argument rather then getting it internally from global state?

I added an arg start_path to address this comment, and set the default value using a global state variable. Not sure if that's the best way to do it, so I'm open to suggestions.

Signed-off-by: claireyywang <[email protected]>

codebot · 2020-10-01T01:42:16Z

Thanks so much! This will be a very nice UX improvement.

dirk-thomas

Those are the only two verbs in colcon-core, and seem to address the feature request in #369. Is there anywhere else that could also use this check?

The same check seems to be applicable to several other verbs: graph, info, list, test-result.

dirk-thomas · 2020-10-01T22:09:37Z

colcon_core/verb/build.py

@@ -116,6 +123,7 @@ def add_arguments(self, *, parser):  # noqa: D102
        self.task_argument_destinations = decorated_parser.get_destinations()

    def main(self, *, context):  # noqa: D102
+        check_and_mark_root_dir(context.args.start_path)


I think the invocation here in the verb is too late since e.g. the log directory has already been created which isn't desired.

colcon_core/verb/__init__.py

colcon_core/verb/build.py

Signed-off-by: claireyywang <[email protected]>

claireyywang · 2020-10-02T00:27:13Z

The same check seems to be applicable to several other verbs: graph, info, list, test-result.

Wouldn't it limit the usability of these verbs if they can only be invoked in root directory?

dirk-thomas · 2020-10-02T20:38:53Z

Wouldn't it limit the usability of these verbs if they can only be invoked in root directory?

Isn't that the whole idea of this PR - to limit the locations where colcon can be invoked to the root of a workspace?

claireyywang · 2020-10-02T21:08:27Z

Isn't that the whole idea of this PR - to limit the locations where colcon can be invoked to the root of a workspace?

I think the consequence of not running build and test at root dir is more significant than all the other verbs, cuz they rely on root level package discovery and create build and install folders even when the process is not successful. Whereas verbs like graph, info ... can be executed in child directories relatively faster and easier without creating substantial overhead. I can also think of use cases like running colcon list within a repo to figure out what packages are available inside, which will not be available is all verbs are limited to root workspace.

The idea of this PR is to address discussion in #139 and #369, which both point to the build verb.

dirk-thomas · 2020-10-02T22:50:35Z

How about test-result which wouldn't work correctly if not invoked in the root?

claireyywang · 2020-10-02T23:22:23Z

How about test-result which wouldn't work correctly if not invoked in the root?

lol I've actually never used that verb. I'll take a look and see if I can add the root dir check to it as well. Thanks for the heads up.

dirk-thomas · 2020-11-04T22:42:03Z

@claireyywang Friendly ping.

Flova · 2024-01-23T14:09:15Z

What is the state of this PR? This as well as #487 are some quite significant UX improvements that people who start out in ROS 2 (coming from ROS 1) find very much annoying. If @claireyywang is not available anymore, I would take a look at it. But I don't want to invest time and effort only for it to end up like #487.

cottsay · 2024-01-23T14:13:05Z

We will address this with a separate colcon extension. There has been discussion in #139. I'll close this PR since this isn't the approach we plan to take.

claireyywang added 5 commits September 22, 2020 14:28

add check root dir

24de2b5

Signed-off-by: claireyywang <[email protected]>

add warning msg

2cbc216

Signed-off-by: claireyywang <[email protected]>

add import fn to test.py

7615574

Signed-off-by: claireyywang <[email protected]>

fix style

6517968

Signed-off-by: claireyywang <[email protected]>

remove blank space and bad quotes

a8f23b5

Signed-off-by: claireyywang <[email protected]>

dirk-thomas reviewed Sep 22, 2020

View reviewed changes

claireyywang added 5 commits September 29, 2020 16:29

address review comment

4017d12

Signed-off-by: claireyywang <[email protected]>

remove pycache

a2b7cf7

Signed-off-by: claireyywang <[email protected]>

address review comments

8a3f68e

Signed-off-by: claireyywang <[email protected]>

remove debug line

1414381

Signed-off-by: claireyywang <[email protected]>

move default_start_path

2bb4250

Signed-off-by: claireyywang <[email protected]>

claireyywang marked this pull request as draft September 30, 2020 00:59

modify start_path

a928198

Signed-off-by: claireyywang <[email protected]>

Merge branch 'master' into claire/check-root-dir

4b65b68

claireyywang marked this pull request as ready for review October 1, 2020 20:33

dirk-thomas added the enhancement New feature or request label Oct 1, 2020

dirk-thomas reviewed Oct 1, 2020

View reviewed changes

change marker file name and remove cli args

fe56301

Signed-off-by: claireyywang <[email protected]>

claireyywang self-assigned this Nov 30, 2020

claireyywang removed their assignment Feb 18, 2021

cottsay closed this Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add check root dir #383

add check root dir #383

claireyywang commented Sep 22, 2020 •

edited

Loading

codecov bot commented Sep 22, 2020 •

edited

Loading

dirk-thomas left a comment

dirk-thomas Sep 22, 2020

claireyywang commented Sep 30, 2020 •

edited

Loading

codebot commented Oct 1, 2020

dirk-thomas left a comment

dirk-thomas Oct 1, 2020

claireyywang commented Oct 2, 2020

dirk-thomas commented Oct 2, 2020

claireyywang commented Oct 2, 2020 •

edited

Loading

dirk-thomas commented Oct 2, 2020

claireyywang commented Oct 2, 2020

dirk-thomas commented Nov 4, 2020

Flova commented Jan 23, 2024

cottsay commented Jan 23, 2024

add check root dir #383

add check root dir #383

Conversation

claireyywang commented Sep 22, 2020 • edited Loading

codecov bot commented Sep 22, 2020 • edited Loading

Codecov Report

dirk-thomas left a comment

Choose a reason for hiding this comment

dirk-thomas Sep 22, 2020

Choose a reason for hiding this comment

claireyywang commented Sep 30, 2020 • edited Loading

codebot commented Oct 1, 2020

dirk-thomas left a comment

Choose a reason for hiding this comment

dirk-thomas Oct 1, 2020

Choose a reason for hiding this comment

claireyywang commented Oct 2, 2020

dirk-thomas commented Oct 2, 2020

claireyywang commented Oct 2, 2020 • edited Loading

dirk-thomas commented Oct 2, 2020

claireyywang commented Oct 2, 2020

dirk-thomas commented Nov 4, 2020

Flova commented Jan 23, 2024

cottsay commented Jan 23, 2024

claireyywang commented Sep 22, 2020 •

edited

Loading

codecov bot commented Sep 22, 2020 •

edited

Loading

claireyywang commented Sep 30, 2020 •

edited

Loading

claireyywang commented Oct 2, 2020 •

edited

Loading