Skip to content

Unpin mypy.#617

Merged
clalancette merged 2 commits intomasterfrom
clalancette/unpin-mypy
Feb 16, 2022
Merged

Unpin mypy.#617
clalancette merged 2 commits intomasterfrom
clalancette/unpin-mypy

Conversation

@clalancette
Copy link
Copy Markdown
Contributor

@clalancette clalancette commented Feb 7, 2022

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

This should fix the warnings we are seeing on Jammy, like https://ci.ros2.org/job/ci_linux/16057/ . On the other hand, I don't know what the situation is going to be like on Foxy and Galactic on Focal, so testing still needs to be done here.

@clalancette
Copy link
Copy Markdown
Contributor Author

clalancette commented Feb 7, 2022 •
edited
Loading

CI using this branch.

Rolling:

  • Focal Linux Build Status
  • Focal Linux-aarch64 Build Status
  • Windows Build Status
  • Jammy Linux Build Status
  • Jammy Linux-aarch64 Build Status

Foxy:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

(if everything above is happy, I'll run Galactic CI tomorrow)

@clalancette
Copy link
Copy Markdown
Contributor Author

clalancette commented Feb 8, 2022

OK, so as suspected this fixed things on Jammy, but broken things for Foxy.

That leaves us with a few options:

  1. Continue to use pip to install mypy (we have to for Windows), but conditionalize the version depending on Focal or Jammy.
  2. Install mypy from distribution packages on Focal and Jammy, and only install from pip for Windows.
  3. Disable the mypy tests for sros2 on Foxy (it's the only thing that is failing).

@nuclearsandwich Opinions?

@nuclearsandwich
Copy link
Copy Markdown
Member

nuclearsandwich commented Feb 8, 2022

Install mypy from distribution packages on Focal and Jammy, and only install from pip for Windows.

If I recall correctly pinning to the version in Focal was the agreed upon solution in the 2021-06-15 ROS 2 meeting but there's no mention there of doing so via system package or pip. I'm in favor of using the debs and rpms in CI in order to more closely replicate the buildfarm environment.

@clalancette
Copy link
Copy Markdown
Contributor Author

clalancette commented Feb 8, 2022

If I recall correctly pinning to the version in Focal was the agreed upon solution in the 2021-06-15 ROS 2 meeting but there's no mention there of doing so via system package or pip. I'm in favor of using the debs and rpms in CI in order to more closely replicate the buildfarm environment.

Yep, agreed. That is my preferred solution as well. I'll try this out and see how it goes.

@nuclearsandwich nuclearsandwich mentioned this pull request Feb 8, 2022
Merged
@clalancette clalancette force-pushed the clalancette/unpin-mypy branch from 924ad6b to 392506a Compare February 8, 2022 23:02
@clalancette
Copy link
Copy Markdown
Contributor Author

clalancette commented Feb 8, 2022 •
edited
Loading

All right, the new code I pushed is more complicated, but also should get us closer to what we want. Let's see how this goes:

Rolling:

  • Focal Linux Build Status
  • Focal Linux-aarch64 Build Status
  • Windows Build Status
  • Jammy Linux Build Status

Foxy:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

(if this goes well, I'll run additional jobs for Galactic and RHEL tomorrow)

@clalancette clalancette mentioned this pull request Feb 10, 2022
Merged
@clalancette clalancette force-pushed the clalancette/unpin-mypy branch from 9839f2e to 25718bf Compare February 15, 2022 21:08
@clalancette
Switch to a different version of mypy depending on distribution.
668c819 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette clalancette force-pushed the clalancette/unpin-mypy branch from 25718bf to 668c819 Compare February 15, 2022 21:40
@clalancette
Copy link
Copy Markdown
Contributor Author

clalancette commented Feb 15, 2022 •
edited by nuclearsandwich
Loading

I've redone this completely, and what I've done should make it much more robust. Let's try another CI to see what happens:

Rolling:

  • Ubuntu Focal Build Status
  • Ubuntu Focal-aarch64 Build Status
  • Windows Build Status
  • Ubuntu Jammy Build Status

Foxy:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette mentioned this pull request Feb 15, 2022
Merged
nuclearsandwich
nuclearsandwich reviewed Feb 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far. Hanging on to the ✔️ until CI comes back happy.

@clalancette @nuclearsandwich
Update ros2_batch_job/__main__.py
f5bd644 
Co-authored-by: Steven! Ragnarök <nuclearsandwich@users.noreply.github.com>
@clalancette clalancette marked this pull request as ready for review February 16, 2022 13:03
@clalancette
Copy link
Copy Markdown
Contributor Author

clalancette commented Feb 16, 2022

CI was really happy with this, so I converted it to a real PR. Please take another look when you get a chance.

@clalancette
Copy link
Copy Markdown
Contributor Author

clalancette commented Feb 16, 2022 •
edited
Loading

Here's CI just up to ament_cmake, to test that the latest variable names don't have a typo. If this is successful, I'm going to go ahead and merge.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • RHEL Build Status

@clalancette clalancette merged commit 2453630 into master Feb 16, 2022
@delete-merged-branch delete-merged-branch bot deleted the clalancette/unpin-mypy branch February 16, 2022 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nuclearsandwich nuclearsandwich nuclearsandwich approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@clalancette @nuclearsandwich