-
Notifications
You must be signed in to change notification settings - Fork 30
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
Conversation
Signed-off-by: Chris Lalancette <[email protected]>
e7cb1e7
to
bf602da
Compare
bf602da
to
da55dfd
Compare
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]>
da55dfd
to
a31b09a
Compare
@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
|
Signed-off-by: Chris Lalancette <[email protected]>
There was a problem hiding this 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).
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
Signed-off-by: Chris Lalancette <[email protected]>
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]>
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. |
And this is now deployed. |
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#74Changes
@vs_version
in the json solo that specifies the VS release