Migrate subsection of vcstools' API for compatibility with bloom by leander-dsouza · Pull Request #43 · ros-infrastructure/vcs2l

leander-dsouza · 2025-08-27T13:18:12Z

Note

Please refer #751 for verifying integration with bloom.

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	N/A
Primary OS tested on	Ubuntu
Is this a breaking change?	No
Does this PR contain AI generated software?	No

Description of contribution in a few bullet points

This ports over four main functions from vcstools, namely export_repositories , checkout, get_vcs_client, and get_path for all the VCS clients.
This is because these are the four prominent methods used in bloom as seen here.
- checkout reference
- export_repositories reference
- get_vcs_client reference
- get_path reference
In addition, the corresponding integration tests have been added to both of these functions across svn, bzr, hg, and git clients.
Breezy has been added to the CI for testing on macOS and Ubuntu.
The integration can be safely verified in #751 at bloom. All the system and unit tests pass upon merge.

Description of how this change was tested

Ran pytest locally to ensure all the unit and linting tests pass:
```
pytest -s -v test
```

Signed-off-by: Leander Stephen Desouza leanderdsouza1234@gmail.com

codecov-commenter · 2025-10-01T05:25:58Z

Codecov Report

❌ Patch coverage is 37.50000% with 145 lines in your changes missing coverage. Please review.
✅ Project coverage is 28.11%. Comparing base (a4f3a63) to head (1ca67bc).

Files with missing lines	Patch %	Lines
vcs2l/clients/svn.py	9.83%	55 Missing ⚠️
vcs2l/clients/bzr.py	5.71%	33 Missing ⚠️
vcs2l/clients/git.py	51.61%	21 Missing and 9 partials ⚠️
vcs2l/clients/hg.py	65.95%	10 Missing and 6 partials ⚠️
vcs2l/clients/__init__.py	63.63%	4 Missing ⚠️
vcs2l/clients/vcs_base.py	50.00%	3 Missing ⚠️
vcs2l/clients/tar.py	60.00%	2 Missing ⚠️
vcs2l/clients/zip.py	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #43      +/-   ##
==========================================
+ Coverage   26.96%   28.11%   +1.14%     
==========================================
  Files          31       31              
  Lines        2240     2465     +225     
  Branches      393      437      +44     
==========================================
+ Hits          604      693      +89     
- Misses       1576     1698     +122     
- Partials       60       74      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

SuperJappie08 · 2025-10-01T14:02:10Z

@leander-dsouza, I noticed that some implementations of export_repository are prefixed with an underscore (_export_repository), is this intentional?

nuclearsandwich

It looks good but for the one comment about the git client which I believe can be simplified dramatically.

nuclearsandwich · 2025-10-03T18:49:48Z

+
+        # If current version matches export version and no local changes, export directly
+        current_sha = self._get_current_version()
+        export_sha = self._get_version_sha(version) if version else current_sha


if we have an exact version to provide as a commit-ish->tree-ish what is the requirement to use a temp directory. git-archive does not make any changes to the working directory. I have not gone spelunking in the history but I'm willing to hypothesize that this behavior is carried over from a vcs which does not support exporting archives of snapshots without checking them out first.

git archive --format=tar.gz --output={filepath} export_sha

Yes, you are correct, Steven.
I tried to replicate the archiving behaviour of the original implementation of the archiving process in vcstools

I have now simplified the implementation of this section, as git archive does not change the cwd.

cottsay · 2025-09-25T16:50:12Z

        result['output'] = branch
        return result

+    def _export_repository(self, version, basepath):


A handful of these are named _export_repository instead of export_repository. to match the API. Is that intentional?

I briefly remember selectively naming the export methods for this reason.
However, I cannot find these methods in the vcstools repository. Hence, I added a commit to standardise the export repository methods across all VCS clients.

leander-dsouza · 2025-10-06T16:01:02Z

@leander-dsouza, I noticed that some implementations of export_repository are prefixed with an underscore (_export_repository), is this intentional?

I remember drafting the export methods to match the vcstools repository. However, I think I was mistaken, and hence I added a commit to standardise all the export repository methods across all VCS clients.

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

KmoM88

Hi @leander-dsouza ,

Thank you for your hard work on this!

I've left a few comments on the files above pointing out some areas where we maybe can make the API even more robust, predictable, and maintainable.

Please take a look at the comments, they are very much open for discussion, and I'd love to hear your thoughts on these approaches.

Great job!

KmoM88 · 2026-06-24T14:29:19Z

+        if os.path.exists(self.path):
+            if os.path.isdir(self.path):
+                if os.listdir(self.path):
+                    return False
+            else:
+                return False


I noticed that the error handling in checkout is inconsistent when the target directory already exists and is not empty:

git.py and hg.py return False.

svn.py and bzr.py raise a RuntimeError.

To keep the API unified and predictable, I recommend standardizing this behavior. Raising a descriptive exception (rather than returning False) would be the most robust approach.

What do you think?

KmoM88 · 2026-06-24T14:30:58Z

+        if os.path.exists(self.path):
+            if os.path.isdir(self.path):
+                # Check if directory is empty
+                if os.listdir(self.path):
+                    return False
+            else:
+                return False


I noticed that the error handling in checkout is inconsistent when the target directory already exists and is not empty:

git.py and hg.py return False.

svn.py and bzr.py raise a RuntimeError.

To keep the API unified and predictable, I recommend standardizing this behavior. Raising a descriptive exception (rather than returning False) would be the most robust approach.

What do you think?

KmoM88 · 2026-06-24T14:57:09Z

+            % self.__class__.type
+        )
+
+    def checkout(self, url, version=None, verbose=False, shallow=False, timeout=None):


The timeout parameter is defined here in the base class, but it isn't currently used in any of the client implementations.

While the underlying command-line tools have inconsistent support for timeout flags, we can actually implement a clean, universal timeout in Python. The run_command helper in this file uses subprocess.Popen, and proc.communicate() has built-in support for a timeout parameter.

Would it be possible to pass timeout down from the clients' checkout methods to run_command and proc.communicate(timeout=timeout)?

Ref: https://docs.python.org/3/library/subprocess.html

proc = subprocess.Popen(...) try: outs, errs = proc.communicate(timeout=15) except TimeoutExpired: proc.kill() outs, errs = proc.communicate()

If implementing this is outside the scope of this PR, we should probably remove the timeout parameter from the base class and client definitions for now to keep the API contract accurate. Let me know what you think.

KmoM88 · 2026-06-24T15:16:39Z

+            result = self._run_command(cmd)
+        except Exception as e:
+            print('Branch command failed with exception: %s' % str(e))
+            return False


Digging deeper into the error handling, I noticed that in the bzr.py checkout method, the try...except block catches a generic Exception, prints it, and then returns False.

This "swallows" the exception, meaning the calling code only knows that something failed but loses all context about why. This can lead to silent failures that are difficult to debug.

Looking at the other clients, this behavior is inconsistent:

git.py and hg.py follow the same problematic pattern of swallowing exceptions.

svn.py has no try...except block at all, making it brittle to unexpected errors.

I have a suggestion for a unified solution:

Instead of managing this in each client, what if we made the shared _run_command method in VcsClientBase responsible for this?

We could wrap the call to the global run_command inside _run_command with a try...except block. If any unexpected exception occurs, _run_command itself could then raise a specific, library-wide exception (e.g., Vcs2lError).

This would give us two major benefits:

Consistency: All clients would automatically have the same robust, exception-based error handling.

Simplicity: We could remove the repetitive try...except blocks from the git, hg, and bzr clients, making their code cleaner.

What are your thoughts on centralizing the exception handling within _run_command this way?

leander-dsouza force-pushed the leander-dsouza/vcstools-port branch 4 times, most recently from 5730d2c to 650fcd3 Compare August 27, 2025 13:58

leander-dsouza force-pushed the leander-dsouza/fix-svn-test branch from 659c3b8 to 5f6ce04 Compare August 27, 2025 14:09

leander-dsouza force-pushed the leander-dsouza/vcstools-port branch 4 times, most recently from 477c4be to d08697b Compare August 27, 2025 14:37

leander-dsouza marked this pull request as ready for review August 27, 2025 14:39

leander-dsouza requested a review from claraberendsen as a August 27, 2025 14:39

leander-dsouza force-pushed the leander-dsouza/fix-svn-test branch from 5f6ce04 to e47aac4 Compare August 27, 2025 15:08

leander-dsouza mentioned this pull request Aug 27, 2025

Enable test run on mercurial and subversion #42

Merged

Base automatically changed from leander-dsouza/fix-svn-test to main August 27, 2025 18:52

leander-dsouza force-pushed the leander-dsouza/vcstools-port branch 2 times, most recently from 8e32c1a to 138ad9f Compare August 28, 2025 12:45

claraberendsen requested a review from cottsay September 8, 2025 16:34

claraberendsen added the enhancement New feature or request label Sep 9, 2025

leander-dsouza force-pushed the leander-dsouza/vcstools-port branch from 138ad9f to 83040dc Compare September 16, 2025 10:36

leander-dsouza marked this pull request as draft September 16, 2025 11:26

leander-dsouza mentioned this pull request Sep 16, 2025

Replaced vcstools with vcs2l. ros-infrastructure/bloom#751

Draft

leander-dsouza marked this pull request as ready for review September 16, 2025 12:29

leander-dsouza changed the title ~~Port Vcstools~~ Migrate subsection of vcstools' API for compatibility with bloom Sep 16, 2025

nuclearsandwich previously approved these changes Oct 3, 2025

View reviewed changes

cottsay reviewed Oct 4, 2025

View reviewed changes

leander-dsouza dismissed nuclearsandwich’s stale review via 5b27149 October 6, 2025 15:57

leander-dsouza force-pushed the leander-dsouza/vcstools-port branch from 4afa4b1 to 5b27149 Compare October 6, 2025 15:57

leander-dsouza self-assigned this Jan 26, 2026

leander-dsouza added 18 commits February 20, 2026 22:16

Added checkout implementation for zip and tar clients.

fd0f0ce

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Added checkout implementation and integration tests for the bzr client.

0a34378

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Added checkout implementation and integration tests for Git client.

fb8af85

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Added checkout implementation and integration tests for svn client.

84eba6c

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Added checkout implementation and integration tests for hg client.

e40bda8

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Added export_repository implementation for zip and tar clients.

717ce48

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Added export_repository implementation for bzr client.

32b148c

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Ignore bzr tests if system is Windows.

9296147

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Added export_repository method to SvnClient and corresponding tests.

4141cb0

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Added export_repository support and corresponding tests for hg client.

c3d1a75

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Added export_repository support and corresponding tests for git client.

00aed88

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Added breezy to the CI for testing across linux and macOS.

7e67f55

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Add helper function to match vcstools API.

eca34f7

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Added get_path method to mimic vcstools' API.

713cefe

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Skip breezy tests in favor of codecov image.

417dda4

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Simplify git client checkout_repository method.

1261d6f

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Standardised all export repository methods across clients.

83139b4

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

Refactor tests to not rely on remote repositories.

1ca67bc

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>

leander-dsouza force-pushed the leander-dsouza/vcstools-port branch from 5b27149 to 1ca67bc Compare February 21, 2026 00:33

claraberendsen requested a review from cottsay February 23, 2026 20:21

claraberendsen mentioned this pull request May 18, 2026

Drop python3-bloom from ros-dev-tools RPM variant ros-infrastructure/infra-variants#9

Merged

KmoM88 reviewed Jun 24, 2026

View reviewed changes

Uh oh!

Conversation

leander-dsouza commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Basic Info

Description of contribution in a few bullet points

Description of how this change was tested

Uh oh!

codecov-commenter commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SuperJappie08 commented Oct 1, 2025

Uh oh!

nuclearsandwich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leander-dsouza commented Oct 6, 2025

Uh oh!

KmoM88 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

leander-dsouza commented Aug 27, 2025 •

edited

Loading

codecov-commenter commented Oct 1, 2025 •

edited

Loading