search

ros-infrastructure / catkin_pkg

Standalone Python library for the catkin build system.
https://github.com/ros/catkin
Other
47 stars 89 forks source link

Respect (AMENT|COLCON|CATKIN)_IGNORE directories #286

Closed rotu closed 4 years ago

rotu commented 4 years ago

This reflects the behavior of colcon, which tests whether the path exists, not whether it is a regular file.

dirk-thomas commented 4 years ago

Looking at different REPs mentioning CATKIN_IGNORE (128, 133, 141) all of them mentioned it to be an empty marker file. So I am not sure if a directory with that name should be considered equal. It could just be that colcons implementation unknowingly diverges from this.

Can you explain why you think these ignore marker should also possibly subdirectories and can't just create a file instead?

rotu commented 4 years ago

My particular use case is admittedly a bit of a hack - because vcstool doesn't like clobbering repos on Windows, I instead install a marker directory inside the repo to ignore: https://github.com/ros2/rmw_cyclonedds/blob/3dc89b885cbdfba8372184cb0e6279b28e6bad7e/.github/resources/suppress_other_rmw.repos

  1. I think it's less surprising for both to work. If a COLCON_IGNORE directory exists but is a directory, it's clear the user's intent is to ignore the parent directory. Also, if COLCON_IGNORE is a directory, then touching that directory file will succeed silently.

  2. It provides some added utility for git users, since git ignores empty directories. So you can create COLCON_IGNORE as a file if it should be checked in and as a directory if it shouldn't.

rotu commented 4 years ago

Also, I would argue that a directory is a type of file. There was just no use for the REPs to get pedantic about it.

dirk-thomas commented 4 years ago

REP 128 uses the description "empty marker file" which imo clearly identifies a file and not a directory. So I think before we can consider this patch the REP(s) need to be updated to clarify that both types should be considered.

rotu commented 4 years ago
  1. By that reading a marker file which is not empty should not be respected. That's insane.
  2. A directory is a file.

There is no need to update the REP, but if you feel there is for some reason, go ahead. Nevermind, did it myself https://github.com/ros-infrastructure/rep/pull/256

gavanderhoorn commented 4 years ago

@rotu: is it your intention to antagonise people?

I'm Dutch and quite used to directness (sometimes even probably called rudeness), but there doesn't seem to be a need to be abrasive in your responses to people.

Why is it insane to understand the sentence is an empty marker file to mean just that: an empty file? A directory is commonly understood to not be a file, that's why it's called a directory, and why there are different ways to manage them and there are different usages for them. It may be that file systems in their implementation consider them similarly, but the very fact that they have different names seem to imply they are not the same for all intents and purposes.

If the REP states that the file should be empty, then yes, that would seem to mean that non-empty files are not considered valid CATKIN_IGNORE files. I'm pretty sure non-empty files are not considered invalid, but I wouldn't call it insane even if they were.

rotu commented 4 years ago

@rotu: is it your intention to antagonise people?

I'm sorry, I got riled up. @dirk-thomas, I felt like you were being willfully stubborn by insisting I open yet another PR for this change. I should have given you the benefit of the doubt, and I apologize that I didn't.

Why is it insane to understand the sentence is an empty marker file to mean just that: an empty file?

As the file is described as "an empty marker file" I interpreted it to mean "the presence of an AMENT_IGNORE file", not its content or properties, is what matters. I do think it's unreasonable to disregard an AMENT_IGNORE file if it's not empty. For instance, if it has a single newline, which text editors may insert.

the very fact that they have different names seem to imply they are not the same for all intents and purposes.

A directory is a file. As per man ls:

NAME
       ls - list directory contents

SYNOPSIS
       ls [OPTION]... [FILE]...

DESCRIPTION
       List information about the FILEs (the current directory by default).

I made that explicit in https://github.com/ros-infrastructure/rep/pull/256.

dirk-thomas commented 4 years ago

A directory is a file. As per man ls:

I assume you are familiar that the terms file and directory are clearly distinguished in a lot of other cases, e.g. in the Python API is_dir / is_file.

The fact that you read this differently than many others is just an indicator that it needs to be explicit / clarified in the REP - as done in ros-infrastructure/rep#256.

rotu commented 4 years ago

I assume you are familiar that the terms file and directory are clearly distinguished in a lot of other cases, e.g. in the Python API is_dir / is_file.

Even the linked documentation concedes that a directory is a file. I suppose "is_regular_file" was just too wordy:

Return True if the path points to a regular file (or a symbolic link pointing to a regular file), False if it points to another kind of file.

dirk-thomas commented 4 years ago

I simplified the logic in the patch: e335bbe569fc7701aeb39c56d65eba1c494ecd9c, adee3472f442bdaa4a036a6ebff12475937aaae0.

With the REP updated I will go ahead and merge (and release) this.