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

Multi Visual Studio version support #786

Merged
merged 16 commits into from
Oct 2, 2024

Conversation

claraberendsen
Copy link
Contributor

@claraberendsen claraberendsen commented Jul 15, 2024

Description

This PR restores the functionality of selecting which Visual Studio version you want the CI to run with.
This is a cherry-pick from the last approach of #784, for more debugging and lot's of test commits you can check the related PR.

This is paired to the changes to ros2-cookbooks in ros-infrastructure/ros2-cookbooks#74

Changes

  • Inserts a new key @vs_version in the json solo that specifies the VS release
  • Modifies on job run the value according to CI_VS_VERSION
  • Updates options to target 2019 and 2022 releases

@clalancette clalancette force-pushed the claraberendsen/multi-vs-support branch 13 times, most recently from e7cb1e7 to bf602da Compare September 3, 2024 19:54
@clalancette clalancette force-pushed the claraberendsen/multi-vs-support branch from bf602da to da55dfd Compare September 3, 2024 19:57
In particular, we add in the ability to switch between
MSVC 2019 and 2022.  To get there requires a lot of
explanation.

First of all, we need to modify the Chef recipes over
at https://github.com/ros-infrastructure/ros2-cookbooks
how to install alternate versions of MSVC and how to
setup the environment for each.  That is done by the
companion PR to this one.

In this PR, we need to tell the Chef recipes which version
to install, and then also kick off the build using the
appropriate environment.

In order to tell the Chef recipes to install, we use a
configuration variable called "vs_release" in the
install_ros_<rosdistro>.json file per ROS distribution.
The value we set for the "vs_release" is always "@vs_version";
that makes substitution later easier.  Then in the job
configuration, we use a string replace to replace the
@vs_version with the actual version of MSVC we want Chef to
install (I attempted to use a regex here, but I ran into
nasty quoting issues between cmd, powershell, and Docker,
and finally gave up).

The other part of this change is to kick off the build using
the appropriate environment.  It turns out that we have been
double-setting up the environment via vcvarsall.bat on Windows
for a long time; once in the Dockerfile, and once in the
ros2_batch_job batch script which setup the environment.  However,
the latter version was buggy, and actually pointed to the incorrect path.

In theory, we could fix either vsvarsall.bat invocation.  In
practice, I ran into (more) nasty issues with being able to
do environmental substitutions on Windows during a Docker
CMD.  To sidestep this completely, we remove the setup of the
environment there, and fix the pathing issues in ros2_batch_job.
This seems to make everything work.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette force-pushed the claraberendsen/multi-vs-support branch from da55dfd to a31b09a Compare September 4, 2024 00:15
@clalancette
Copy link
Contributor

clalancette commented Sep 4, 2024

@claraberendsen @nuclearsandwich I think I finally, finally figured out all of the complicated issues going on with this PR. Please see the long explanation in a31b09a for why everything here is where it is. I would very much appreciate a look from the both of you, since I'd like to get this in to unblock @ahcorde .

Since this PR only touches the Windows jobs, I'm only going to run CI for it. But I'm going to run quite a lot of different CI here. Note that all of these jobs will be run using the test_ci_windows job, since that is the one that has the changes for Windows in the job (once this is approved, I'll obviously do a full deploy).

  • Rolling Windows Release 2019 Build Status
  • Rolling Windows Release 2022 (expected to start, but fail compiling, since we need further changes) Build Status
  • Rolling Windows Debug 2019 Build Status
  • Rolling Windows Debug 2022 (expected to start, but fail compiling, since we need further changes) Build Status
  • Jazzy Windows Release 2019 Build Status
  • Jazzy Windows Debug 2019 Build Status
  • Iron Windows Release 2019 Build Status
  • Iron Windows Debug 2019 Build Status
  • Humble Windows Release 2019 Build Status
  • Humble Windows Debug 2019 Build Status

Signed-off-by: Chris Lalancette <[email protected]>
Copy link
Contributor Author

@claraberendsen claraberendsen left a comment

Choose a reason for hiding this comment

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

I don't seem to have permission to officially approve this PR, but it looks good to me.
Thanks @clalancette for taking this up and pushing it to the finish line, I would like to get @nuclearsandwich review in before merging to have a fresh set of eyes that has not been hammering at this issue before to catch anything I might have missed (I might be too eager to not look at this for some time).

.gitmodules Outdated Show resolved Hide resolved
Comment on lines +48 to +49
vs = self.args.visual_studio_version
f.write(f'call "C:\\Program Files (x86)\\Microsoft Visual Studio\\{vs}\\BuildTools\\VC\\Auxiliary\\Build\\vcvarsall.bat" x86_amd64' + os.linesep)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a fair compromise and removes the problem of docker CMD execution string expansion that has weird and unexpected behavior.

windows_docker_resources/Dockerfile Show resolved Hide resolved
Copy link
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.

A few minor suggestions but otherwise this is ready to go!

job_templates/ci_job.xml.em Outdated Show resolved Hide resolved
job_templates/ci_job.xml.em Outdated Show resolved Hide resolved
job_templates/packaging_job.xml.em Outdated Show resolved Hide resolved
job_templates/packaging_job.xml.em Outdated Show resolved Hide resolved
job_templates/packaging_job.xml.em Outdated Show resolved Hide resolved
job_templates/ci_job.xml.em Outdated Show resolved Hide resolved
Signed-off-by: Chris Lalancette <[email protected]>
First, set 2022 *after* 2019, so that 2019 remains the
default for now.

Second, make sure to use @@ in the job_templates for the
deploy, otherwise create_jenkins_jobs.py barfs when trying
to do a substitution via em.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor

I did a test deploy to https://ci.ros2.org/view/All/job/test_ci_windows/ , and then ran two jobs:

As expected, the 2019 one completed, and the 2022 one failed to compile. That's because we don't have our dependencies setup for 2022 yet.

With that, I'm going to go ahead and merge this in and deploy it for real.

@clalancette clalancette merged commit 15d614f into master Oct 2, 2024
1 check passed
@clalancette clalancette deleted the claraberendsen/multi-vs-support branch October 2, 2024 12:43
@clalancette
Copy link
Contributor

And this is now deployed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants