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.

Sign up for GitHub

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

Jump to bottom

[CI] Use windows docker image in CI. #4426

Merged
merged 2 commits into from
May 16, 2021
Merged

[CI] Use windows docker image in CI. #4426

merged 2 commits into from
May 16, 2021

Conversation

larshg
Copy link
Contributor

@larshg larshg commented Sep 26, 2020

No description provided.

@larshg larshg force-pushed the AddWindowsContainers branch 9 times, most recently from 8930f12 to 43526be Compare September 27, 2020 15:41
@kunaltyagi
Copy link
Member

Perhaps, add a dockerfile and a CI to build the image so you can use PCL's dockerhub to maintain the images

@larshg
Copy link
Contributor Author

larshg commented Sep 27, 2020

Yeah, I'll do that when it works. Currently it fail to access c:\__a\bin and I have no clue why it goes there. And can't find anyone else having the same problem...

@larshg larshg force-pushed the AddWindowsContainers branch 5 times, most recently from f21d0c1 to c60b511 Compare September 29, 2020 10:18
@larshg larshg marked this pull request as ready for review September 29, 2020 11:19
@larshg larshg mentioned this pull request Oct 1, 2020
Closed
@larshg
Copy link
Contributor Author

larshg commented Oct 3, 2020

So how do I go about updating the build YAML - should I add a new job as the windows container has to be build on a windows vmImage?

@kunaltyagi
Copy link
Member

So how do I go about updating the build YAML

Modify the .ci/azure-pipelines/env.yaml to add a new job (with windows as the pool's vmImage) to build the windows docker image.

@stale
Copy link

stale bot commented Dec 19, 2020

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

@stale stale bot added the status: stale label Dec 19, 2020
@larshg larshg force-pushed the AddWindowsContainers branch from c60b511 to 5323808 Compare March 18, 2021 06:20
@stale stale bot removed the status: stale label Mar 18, 2021
@larshg larshg force-pushed the AddWindowsContainers branch from 5323808 to a944154 Compare April 2, 2021 17:39
kunaltyagi
kunaltyagi reviewed Apr 6, 2021
View reviewed changes
cmake/pcl_options.cmake Outdated Show resolved Hide resolved
@larshg larshg force-pushed the AddWindowsContainers branch from 463f978 to 1ad9414 Compare April 7, 2021 10:46
@larshg larshg force-pushed the AddWindowsContainers branch from e0af4b0 to 4b31468 Compare April 16, 2021 17:33
@larshg larshg force-pushed the AddWindowsContainers branch from 4b31468 to f2c2362 Compare April 29, 2021 17:41
@larshg larshg requested review from kunaltyagi and mvieth April 29, 2021 20:53
kunaltyagi
kunaltyagi previously approved these changes Apr 29, 2021
View reviewed changes
Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

Concerns due to move from Visual Studio 2017 to 2019:

  • Is it because of the use of windows 2019 container OS?
  • Is the 2019 version the "default" one used now? (I don't know how versions are maintained for VS)
  • If we are dropping support for the older versions, we might want to check if we have some hacks for vs2017 or earlier

Rest LGTM.

We can finally compile visualization on Windows (in same time as before) 🎉 🥳

@larshg
Copy link
Contributor Author

larshg commented Apr 30, 2021

Concerns due to move from Visual Studio 2017 to 2019:

  • Is it because of the use of windows 2019 container OS?
  • Is the 2019 version the "default" one used now? (I don't know how versions are maintained for VS)
  • If we are dropping support for the older versions, we might want to check if we have some hacks for vs2017 or earlier

Rest LGTM.

We can finally compile visualization on Windows (in same time as before) 🎉 🥳

  • No, it was probably due to the tutorials I found, used the newer version, but we can probably install VS2017 if we want to.
  • If you search for Visual Studio, you will be directed to a 2019 download. There is nothing default preinstalled.
  • Right now we don't "support" VS2019, so either one is unsupported, unless we install one of them with VS2017 and the other with VS2019.

VS2019 seems also to be overrepresented in many of the relatively new opened windows issues.

mvieth
mvieth reviewed May 1, 2021
View reviewed changes
test/CMakeLists.txt Outdated Show resolved Hide resolved
kunaltyagi
kunaltyagi previously approved these changes May 6, 2021
View reviewed changes
Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

Looks quite wonderful

mvieth
mvieth previously approved these changes May 6, 2021
View reviewed changes
@mvieth
Copy link
Member

mvieth commented May 13, 2021

@larshg Merge?
Maybe the title could be changed

@larshg larshg changed the title [CI] Add windows container. [CI] Use windows docker image in CI. May 13, 2021
@larshg
Copy link
Contributor Author

larshg commented May 13, 2021

Sound more reasonable?

larshg
larshg commented May 13, 2021
View reviewed changes
test/CMakeLists.txt Outdated Show resolved Hide resolved
@larshg larshg dismissed stale reviews from mvieth and kunaltyagi via ac76e49 May 13, 2021 19:51
kunaltyagi
kunaltyagi previously approved these changes May 13, 2021
View reviewed changes
@kunaltyagi kunaltyagi added changelog: new feature Meta-information for changelog generation module: ci labels May 13, 2021
mvieth
mvieth previously approved these changes May 15, 2021
View reviewed changes
@larshg larshg dismissed stale reviews from mvieth and kunaltyagi via 76cd5c8 May 15, 2021 20:45
@larshg larshg force-pushed the AddWindowsContainers branch from ac76e49 to 76cd5c8 Compare May 15, 2021 20:45
@larshg
Copy link
Contributor Author

larshg commented May 15, 2021

Rebased and squashed a few commits. So should be fine to 🚀 when CI is green.

@larshg larshg merged commit 6881d42 into PointCloudLibrary:master May 16, 2021
@larshg larshg deleted the AddWindowsContainers branch May 16, 2021 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kunaltyagi kunaltyagi kunaltyagi approved these changes

@mvieth mvieth mvieth approved these changes

Assignees
No one assigned
Labels
changelog: new feature Meta-information for changelog generation module: ci
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@larshg @kunaltyagi @mvieth