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

ament_black: 0.2.2-2 in 'iron/distribution.yaml' [bloom] #38753

Closed
wants to merge 1 commit into from

Conversation

nachovizzo
Copy link
Contributor

Increasing version of package(s) in repository ament_black to 0.2.2-2:

ament_black

* Nacho/fix ci tests (#5 <https://github.com/botsandus/ament_black/issues/5>)
  * Add missing test dependency
  * pre-commit
  * Add pre-commit workflow
  * Add CLI test to make sure the package has been properly installed
  * Remove ament hooks
  * Add missing apt-python dependency
  Not sure if it's necessary ...
  * Fix CI pre-commit
  The CI error was fixed in: https://github.com/psf/black/issues/2964
  Thus, we need black >22.3.0
* Contributors: Ignacio Vizzo

ament_cmake_black

* Nacho/fix ci tests (#5 <https://github.com/botsandus/ament_black/issues/5>)
  * Add missing test dependency
  * pre-commit
  * Add pre-commit workflow
  * Add CLI test to make sure the package has been properly installed
  * Remove ament hooks
  * Add missing apt-python dependency
  Not sure if it's necessary ...
  * Fix CI pre-commit
  The CI error was fixed in: https://github.com/psf/black/issues/2964
  Thus, we need black >22.3.0
* Contributors: Ignacio Vizzo

@github-actions github-actions bot added the iron Issue/PR is for the ROS 2 Iron distribution label Oct 11, 2023
@nachovizzo
Copy link
Contributor Author

Guys, how can I locally test this? I'm running the tests in a completely isolated docker environment with this package is the only one there, colcon build and colcon test are passing...

What I'm doing wrong?

Thanks in advance!

@quarkytale
Copy link
Contributor

Hi @nachovizzo, you can run the tests that CI is running on your local machine by following https://github.com/ros/rosdistro/blob/master/CONTRIBUTING.md#unit-testing, is that what you wanted to test?

@nachovizzo
Copy link
Contributor Author

@quarkytale, thanks for the fast reply.

Not really, the package tests that the build farm would run. For example, I missed adding ament_xmllint in the pcakage.xml and the test failed, but locally was working (I prefer the CI behavior). But would like to reproduce this locally

(Stacktrace
ImportError while importing test module '/tmp/ws/src/ament_black/ament_black/test/test_xmllint.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.10/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
test/test_xmllint.py:16: in <module>
    from ament_xmllint.main import main
E   ModuleNotFoundError: No module named 'ament_xmllint'

NOTE: This test-issue was address in botsandus/ament_black@22b53a0 But I'm still waiting on the ros farm to agree on this

@quarkytale
Copy link
Contributor

Before we move further, need to get #38736 (comment) addressed.

@quarkytale quarkytale added the more information needed Maintainers have asked for additional information label Oct 12, 2023
@nachovizzo nachovizzo marked this pull request as draft October 12, 2023 00:23
@nachovizzo
Copy link
Contributor Author

I would like to merge #38754 before letting this go further ;)

@nachovizzo
Copy link
Contributor Author

Before we move further, need to get #38736 (comment) addressed.

Done: #38736 (comment), I would like to proceed with #38754

@tfoote
Copy link
Member

tfoote commented Oct 13, 2023

If you're adding that new dependency then you're should rerun Bloom after tagging a new release. Which will render this pr irrelevant.

In general when you compile in your local workspace you get all the system dependencies that you have on your system. If you want to test your dependency declaration you need to use a stripped down environment that only installs what your package asks for. This is what ci does, but in general it's too high an overhead for day to day development for most users. The ci solution isn't perfect, it still only creates one environment per repo so peer/neighboring packages can mask missing dependencies. But the individual final builds catch that the first time you release and maintainers rarely get caught by that again. So it's not worth the effort of spending potentially N times extra on the ci time for full isolation, and developers would prefer the faster response time.

@nachovizzo
Copy link
Contributor Author

@tfoote, thanks a lot for the info! Re-opening a new PR with a new bumped version ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iron Issue/PR is for the ROS 2 Iron distribution more information needed Maintainers have asked for additional information
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants