Update build type to release & remove multi-step build in CI by shrijitsingh99 · Pull Request #3745 · PointCloudLibrary/pcl

shrijitsingh99 · 2020-03-13T09:28:28Z

Resolves #3656.
Edit: Changed build type for all OS and removed the multi-step builds since there were no longer any OOM issues.

shrijitsingh99 · 2020-03-13T09:39:27Z

If we go ahead with this, no reason not to implement it across all the pipelines.

kunaltyagi · 2020-03-13T11:24:12Z

I want to see the impact on compile times. If there's none or little, Release should be used on all.

So far, looks like not using DebugInfo actually reduced the compile times

SergioRAgostinho

@kunaltyagi Feel free to merge once you're happy with the compile times.

taketwo

Apply to Ubuntu 16.04 as well?

shrijitsingh99 · 2020-03-13T20:40:44Z

I want to see the impact on compile times. If there's none or little, Release should be used on all.

So far, looks like not using DebugInfo actually reduced the compile times

Tested it out 2 separate configurations, Release is around 1.4x faster.

Config 1:
Release: ninja 10032.33s user 701.35s system 2982% cpu 5:59.85 total
RelWithDebInfo: ninja 14415.01s user 911.54s system 2880% cpu 8:52.17 total

Config 2:
Release: make -j12 14019.48s user 853.48s system 592% cpu 41:52.21 total
RelWithDebInfo: make -j12 18437.76s user 1130.40s system 567% cpu 57:26.84 total

Apply to Ubuntu 16.04 as well?

Should I do that in a separate PR since we can apply the same thing to macOS and Windows as well?

taketwo · 2020-03-14T06:53:33Z

I'd switch all jobs (including tutorials) to release mode at the same time in this PR. Just change the title accordingly ;)

Edit: also rebase on the current HEAD to get updated MacOS jobs.

shrijitsingh99 · 2020-03-14T17:02:18Z

I have been testing out the integration with Ubuntu 16 and noticed the flag PCL_WARNINGS_ARE_ERRORS=ON is set and is causing the build to fail, most likely due to the extra warnings generated by the -O3 flag. Is this flag needed since it is not set in Ubuntu 18?

taketwo · 2020-03-14T20:41:37Z

Yes. It is our goal to eventually have PCL_WARNINGS_ARE_ERRORS=ON set for all CI jobs. #3379 tracks the progress.

most likely due to the extra warnings generated by the -O3 flag

Are there many warnings and they trivial to fix? If so, it would be nice to do so and switch to Release in 16.04 job. If that requires more work, then let's leave 16.04 out for now.

shrijitsingh99 · 2020-03-17T10:33:28Z

I'd switch all jobs (including tutorials) to release mode at the same time in this PR. Just change the title accordingly ;)

Edit: also rebase on the current HEAD to get updated MacOS jobs.

Done that. Merged all the CI changes into this PR.

Yes. It is our goal to eventually have PCL_WARNINGS_ARE_ERRORS=ON set for all CI jobs. #3379 tracks the progress.

most likely due to the extra warnings generated by the -O3 flag

Are there many warnings and they trivial to fix? If so, it would be nice to do so and switch to Release in 16.04 job. If that requires more work, then let's leave 16.04 out for now.

There were not many warnings, so I was able to fix them. They need a review.

shrijitsingh99 · 2020-03-17T18:12:26Z

Any reason why only the ubuntu builds have .yaml extension, whereas rest have .yml?

SergioRAgostinho · 2020-03-17T18:26:48Z

Any reason why only the ubuntu builds have .yaml extension, whereas rest have .yml?

I remember windows having troubles with non 3 char extensions back in the days. Not sure what's the current status on this. It was most likely a random inconsistency we overlooked. Ideally, yaml should be used. source

kunaltyagi

🚀 (yml -> yaml can be here or elsewhere)

Release doesn't make MSVC x64 happier. It stills OOMs out.

LGTM

shrijitsingh99 · 2020-03-18T05:45:46Z

I remember windows having troubles with non 3 char extensions back in the days. Not sure what's the current status on this. It was most likely a random inconsistency we overlooked. Ideally, yaml should be used. source

Yeah, older versions of Windows had this problem, don't think its there anymore. WIll test out and rename if it works.

shrijitsingh99 · 2020-03-18T05:47:05Z

🚀 (yml -> yaml can be here or elsewhere)

Release doesn't make MSVC x64 happier. It stills OOMs out.

Yeah, not sure was building fine before.

kunaltyagi · 2020-03-18T06:24:22Z

MSVC is a weird camper. I have manually restarted the job so many times only for the jobs that OOMed out to be successful (most likely due to variation caused by the parallel build, which isn't as managed per target as the unix ones are)

kunaltyagi

Is the CI hardcoded to the file names? The renamed files didn't run on the CI.

Changeset LGTM

shrijitsingh99 · 2020-03-19T04:50:07Z

Is the CI hardcoded to the file names? The renamed files didn't run on the CI.

Changeset LGTM

Yup, they are hardcoded for filenames so they will need to be updated.

taketwo · 2020-03-19T10:04:09Z

Yup, they are hardcoded for filenames so they will need to be updated.

Will do once this is merged.

SergioRAgostinho

LGTM. Despite the messed up casts :')

In terms of commit organization I would squash things into the following 4 topics:

Update build type to release
Removing of multistep build
File renaming
All commits related to suppressing warnings.

kunaltyagi · 2020-03-23T03:37:52Z

Pinging @shrijitsingh99

Using Release occupies less space and is also around 1.4x faster compared to RelWithDebInfo

Compiling some of the test targets with -j2 option used to lead to pipeline failures but now that issue is no longer present

* leaf_node may have been uninitialized * signed overflow may have occurred for m_count & m_capacity

shrijitsingh99 · 2020-03-23T12:14:35Z

LGTM. Despite the messed up casts :')

In terms of commit organization I would squash things into the following 4 topics:

Update build type to release

Removing of multistep build

File renaming

All commits related to suppressing warnings.

Done

taketwo · 2020-03-23T12:52:25Z

Updated the filenames on Azure 🚀

shrijitsingh99 mentioned this pull request Mar 13, 2020

[Ubuntu 19.10 CI] Out of memory (disk space) #3656

Closed

SergioRAgostinho reviewed Mar 13, 2020

View reviewed changes

Comment thread .ci/azure-pipelines/build-ubuntu-19-10.yaml

shrijitsingh99 force-pushed the ubuntu-ci branch from 9d6da81 to 7721d68 Compare March 13, 2020 09:34

shrijitsingh99 requested a review from SergioRAgostinho March 13, 2020 09:38

SergioRAgostinho approved these changes Mar 13, 2020

View reviewed changes

taketwo approved these changes Mar 13, 2020

View reviewed changes

taketwo added the module: ci label Mar 13, 2020

shrijitsingh99 force-pushed the ubuntu-ci branch from 7721d68 to 5ce24ba Compare March 17, 2020 10:23

shrijitsingh99 changed the title ~~Fix Ubuntu 19.10 CI out of memory (disk space) error~~ Update build type to release & remove multi-step build in CI Mar 17, 2020

shrijitsingh99 marked this pull request as ready for review March 17, 2020 10:33

shrijitsingh99 requested a review from taketwo March 17, 2020 10:33

SergioRAgostinho mentioned this pull request Mar 17, 2020

[ci] Addition of osx catalina in CI appears to have disabled ubuntu 19.10 #3757

Closed

SergioRAgostinho self-requested a review March 17, 2020 11:54

kunaltyagi previously requested changes Mar 17, 2020

View reviewed changes

Comment thread .ci/azure-pipelines/build-windows.yml Outdated

Comment thread .ci/azure-pipelines/build-windows.yml

Comment thread .ci/azure-pipelines/build-macos.yml

Comment thread octree/include/pcl/octree/octree_base.h Outdated

shrijitsingh99 force-pushed the ubuntu-ci branch from c9b6ba6 to 1aa0440 Compare March 17, 2020 17:26

shrijitsingh99 force-pushed the ubuntu-ci branch from 1aa0440 to c350800 Compare March 17, 2020 19:09

kunaltyagi reviewed Mar 18, 2020

View reviewed changes

shrijitsingh99 mentioned this pull request Mar 18, 2020

Use matrix in CI for Linux builds #3595

Closed

SergioRAgostinho reviewed Mar 18, 2020

View reviewed changes

Comment thread surface/include/pcl/surface/3rdparty/opennurbs/opennurbs_array.h Outdated

Comment thread surface/include/pcl/surface/3rdparty/opennurbs/opennurbs_array.h Outdated

SergioRAgostinho added the needs: author reply Specify why not closed/merged yet label Mar 18, 2020

kunaltyagi reviewed Mar 19, 2020

View reviewed changes

shrijitsingh99 force-pushed the ubuntu-ci branch from 9d94eed to 58a56c1 Compare March 19, 2020 05:31

kunaltyagi reviewed Mar 19, 2020

View reviewed changes

Comment thread surface/include/pcl/surface/3rdparty/opennurbs/opennurbs_array_defs.h Outdated

taketwo approved these changes Mar 19, 2020

View reviewed changes

SergioRAgostinho self-requested a review March 19, 2020 13:27

SergioRAgostinho approved these changes Mar 19, 2020

View reviewed changes

shrijitsingh99 added 4 commits March 23, 2020 17:35

Update build type to release

cf54d1e

Using Release occupies less space and is also around 1.4x faster compared to RelWithDebInfo

Remove multi-step build

f39996b

Compiling some of the test targets with -j2 option used to lead to pipeline failures but now that issue is no longer present

Rename file extensions from .yml to .yaml

8b77bee

Fix warnings due to -O3 flag

1d1fb6b

* leaf_node may have been uninitialized * signed overflow may have occurred for m_count & m_capacity

shrijitsingh99 force-pushed the ubuntu-ci branch from e6c13c8 to 1d1fb6b Compare March 23, 2020 12:12

SergioRAgostinho removed the needs: author reply Specify why not closed/merged yet label Mar 23, 2020

SergioRAgostinho merged commit d0f5b23 into PointCloudLibrary:master Mar 23, 2020

Uh oh!

Conversation

shrijitsingh99 commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

shrijitsingh99 commented Mar 13, 2020

Uh oh!

kunaltyagi commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergioRAgostinho left a comment

Choose a reason for hiding this comment

Uh oh!

taketwo left a comment

Choose a reason for hiding this comment

Uh oh!

shrijitsingh99 commented Mar 13, 2020

Uh oh!

taketwo commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shrijitsingh99 commented Mar 14, 2020

Uh oh!

taketwo commented Mar 14, 2020

Uh oh!

shrijitsingh99 commented Mar 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shrijitsingh99 commented Mar 17, 2020

Uh oh!

SergioRAgostinho commented Mar 17, 2020

Uh oh!

kunaltyagi left a comment

Choose a reason for hiding this comment

Uh oh!

shrijitsingh99 commented Mar 18, 2020

Uh oh!

shrijitsingh99 commented Mar 18, 2020

Uh oh!

kunaltyagi commented Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kunaltyagi left a comment

Choose a reason for hiding this comment

Uh oh!

shrijitsingh99 commented Mar 19, 2020

Uh oh!

Uh oh!

taketwo commented Mar 19, 2020

Uh oh!

SergioRAgostinho left a comment

Choose a reason for hiding this comment

Uh oh!

kunaltyagi commented Mar 23, 2020

Uh oh!

shrijitsingh99 commented Mar 23, 2020

Uh oh!

taketwo commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shrijitsingh99 commented Mar 13, 2020 •

edited

Loading

kunaltyagi commented Mar 13, 2020 •

edited

Loading

taketwo commented Mar 14, 2020 •

edited

Loading

kunaltyagi commented Mar 18, 2020 •

edited

Loading

taketwo commented Mar 23, 2020 •

edited

Loading