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

Simplify Ubuntu CI using matrix strategy #3783

Merged
merged 2 commits into from
Mar 25, 2020

Conversation

shrijitsingh99
Copy link
Contributor

@shrijitsingh99 shrijitsingh99 commented Mar 23, 2020

As per discussions in #3595
Currently using private docker images, will update it to PCL images before PR as PCL images lack clang currently.
Ubuntu CI: Link

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.

🚀

Note: Needs Azure maintainer to

  1. update the docker image
  2. change the filename for the Ubuntu pipeline

before PR

PS: You need to change the docker image to the official one (it wouldn't fail since it doesn't run, how fun)

@kunaltyagi kunaltyagi added changelog: enhancement Meta-information for changelog generation module: ci labels Mar 23, 2020
@kunaltyagi kunaltyagi changed the title Add matrix strategy in Ubuntu CI Simplify Ubuntu CI using matrix strategy Mar 23, 2020
@shrijitsingh99
Copy link
Contributor Author

shrijitsingh99 commented Mar 23, 2020

PS: You need to change the docker image to the official one (it wouldn't fail since it doesn't run, how fun)

Have done it for now. If more changes are needed and have to be tested will do it on another branch with private images.

@kunaltyagi kunaltyagi added the needs: code review Specify why not closed/merged yet label Mar 23, 2020
@taketwo
Copy link
Member

taketwo commented Mar 25, 2020

Clang is disabled for the time being, so we don't need to update the Docker image. Could you update the PR to switch back to the official images? Then we merge and I update Azure?

@shrijitsingh99
Copy link
Contributor Author

Clang is disabled for the time being, so we don't need to update the Docker image. Could you update the PR to switch back to the official images? Then we merge and I update Azure?

Have already switched back to official images in 2fd357b. Should i remove clang from the Dockerfile too?

Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

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

I'm fine having clang in the Docker image, will make future experiments easier without the need to switch to private Dockerhub.

.ci/azure-pipelines/build-ubuntu.yaml Outdated Show resolved Hide resolved
Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

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

LGTM, please resolve the conflict and squash everything into one or two commits, as you prefer.

To prevent overloading the CI, temporarily disable until better solution is available
@shrijitsingh99
Copy link
Contributor Author

LGTM, please resolve the conflict and squash everything into one or two commits, as you prefer.

Done

@taketwo taketwo merged commit 373927f into PointCloudLibrary:master Mar 25, 2020
@taketwo taketwo removed the needs: code review Specify why not closed/merged yet label Mar 25, 2020
@taketwo
Copy link
Member

taketwo commented Mar 25, 2020

Merged and updated Azure settings. Could you please send a follow-up PR updating the corresponding badge in the README.md?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Meta-information for changelog generation module: ci
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants