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

[BUILD] OTLP HTTP Exporter has build warnings in maintainer mode #1943

Merged
merged 9 commits into from
Feb 3, 2023
Merged

[BUILD] OTLP HTTP Exporter has build warnings in maintainer mode #1943

merged 9 commits into from
Feb 3, 2023

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Jan 30, 2023 •
edited
Loading

Fixes #1942

Changes

Fixed build warnings in the OTLP HTTP exporter.
Added OTLP HTTP to the MAINTAINER_MODE CI.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@marcalff marcalff requested a review from a team January 30, 2023 17:43
@codecov
Copy link

codecov bot commented Jan 30, 2023 •
edited
Loading

Codecov Report

Merging #1943 (43b1124) into main (d56a5c7) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1943   +/-   ##
=======================================
  Coverage   87.12%   87.12%           
=======================================
  Files         165      165           
  Lines        4596     4596           
=======================================
  Hits         4004     4004           
  Misses        592      592
Impacted Files Coverage Δ
...ntelemetry/sdk/metrics/state/sync_metric_storage.h 87.50% <100.00%> (ø)

lalitb
lalitb reviewed Jan 30, 2023
View reviewed changes
ci/install_protobuf.sh Outdated Show resolved Hide resolved
owent
owent reviewed Feb 1, 2023
View reviewed changes
ci/install_protobuf.sh
@@ -38,5 +38,5 @@ wget https://github.com/google/protobuf/releases/download/v${PROTOBUF_VERSION}/p
tar zxf protobuf-cpp-${CPP_PROTOBUF_VERSION}.tar.gz --no-same-owner
cd protobuf-${CPP_PROTOBUF_VERSION}
./configure
make -j ${nproc} && make install
make && make install
Copy link
Member

Choose a reason for hiding this comment

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

We may use make -j || make && make install to reduce the compiling time.

Copy link
Member Author

@marcalff marcalff Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @owent

This part changed a bit back and forth, not sure which commit you looked at.

Currently the code is make -j $(nproc) && make install

When using make -j ${nproc}, which was wrong because there is no nproc environment variable, make -j was executed alone ... this killed the github CI.

make -j $(nproc), which is used in different places already, correctly invokes the nproc utility to return the number of cores. This works fine in CI now.

So, parallel make is used already, precisely to reduce the compiling time.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation.LGTM now.

@lalitb lalitb merged commit 32af352 into open-telemetry:main Feb 3, 2023
@marcalff marcalff deleted the fix_warnings_otlp_1942 branch July 4, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owent owent owent approved these changes

@lalitb lalitb lalitb approved these changes

@ThomsonTan ThomsonTan ThomsonTan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUILD] OTLP HTTP Exporter has build warnings in maintainer mode
4 participants
@marcalff @ThomsonTan @lalitb @owent