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

Upgrade clang-format to 10.0.0, run ./tools/format.sh #544

Merged
merged 1 commit into from
Feb 1, 2021
Merged

Upgrade clang-format to 10.0.0, run ./tools/format.sh #544

merged 1 commit into from
Feb 1, 2021

Conversation

bogdandrutu
Copy link
Member

Signed-off-by: Bogdan Drutu [email protected]

@bogdandrutu bogdandrutu requested a review from a team January 28, 2021 22:40
@bogdandrutu bogdandrutu mentioned this pull request Jan 28, 2021
Merged
@bogdandrutu bogdandrutu force-pushed the runformat branch 2 times, most recently from 28732e6 to 6034164 Compare January 28, 2021 22:48
@codecov
Copy link

codecov bot commented Jan 28, 2021 •
edited
Loading

Codecov Report

Merging #544 (b50726e) into main (2836677) will increase coverage by 0.01%.
The diff coverage is 92.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
+ Coverage   94.32%   94.33%   +0.01%     
==========================================
  Files         197      197              
  Lines        8803     8801       -2     
==========================================
- Hits         8303     8302       -1     
+ Misses        500      499       -1
Impacted Files Coverage Δ
.../include/opentelemetry/common/key_value_iterable.h 100.00% <ø> (ø)
api/include/opentelemetry/nostd/mpark/variant.h 96.71% <ø> (ø)
api/include/opentelemetry/plugin/factory.h 0.00% <ø> (ø)
...ude/opentelemetry/exporters/ostream/log_exporter.h 100.00% <ø> (ø)
...ntelemetry/ext/http/client/curl/http_client_curl.h 82.07% <ø> (ø)
...nclude/opentelemetry/ext/http/client/http_client.h 95.45% <ø> (ø)
sdk/src/logs/logger.cc 65.90% <40.00%> (ø)
...ude/opentelemetry/common/key_value_iterable_view.h 100.00% <100.00%> (ø)
api/include/opentelemetry/nostd/unique_ptr.h 100.00% <100.00%> (ø)
...pentelemetry/trace/span_context_kv_iterable_view.h 84.21% <100.00%> (ø)
... and 10 more

@bogdandrutu bogdandrutu force-pushed the runformat branch 2 times, most recently from b22d31b to e553734 Compare January 28, 2021 23:14
lalitb
lalitb reviewed Jan 29, 2021
View reviewed changes
ci/install_format_tools.sh Outdated
@@ -2,7 +2,8 @@

set -e

apt install -y clang-format-8 python3-pip git curl
apt-get update
apt-get install -y clang-format-11 python3-pip git curl
Copy link
Member

Choose a reason for hiding this comment

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

Nit: format.sh also needs to be modified to ensure right version is picked in case there are multiple versions installed.

Copy link
Member Author

Choose a reason for hiding this comment

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

So far I was not able to find the package to install... Some help would be appreciated

Copy link
Member

@lalitb lalitb Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Which OS ? I can see multiple clang-format versions for ubuntu-latest in default repo:

root@e4c04138cab4:/# apt-cache search clang-format
arcanist-clang-format-linter - clang-format linter for Arcanist
clang-format - Tool to format C/C++/Obj-C code
clang-format-10 - Tool to format C/C++/Obj-C code
clang-format-6.0 - Tool to format C/C++/Obj-C code
clang-format-7 - Tool to format C/C++/Obj-C code
clang-format-8 - Tool to format C/C++/Obj-C code
clang-format-9 - Tool to format C/C++/Obj-C code
clang-format-11 - Tool to format C/C++/Obj-C code
``

Copy link
Member Author

Choose a reason for hiding this comment

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

See the github action failing message:

Reading state information...
E: Unable to locate package clang-format-11

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Missed out the failure.
ubuntu-18.04 and ubuntu-20.04 virtual machines in github doesn't bring clang-format-11 :
https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md : Clang 8.0.1, 9.0.1, 10.0.0
https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu1804-README.md : Clang 6.0.0, 8.0.0, 9.0.0

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest we use the one available in apt-cache with highest version (clang-format-10?), as we usually need run clang-format locally so no need to complicate it with building from source or install binary package.

Copy link
Member

Choose a reason for hiding this comment

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

Agree

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the best long term option is to have a Docker container with all this formating/linting tools that everyone can use

Copy link
Member Author

Choose a reason for hiding this comment

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

See open-telemetry/build-tools#28 for the docker image and instructions to run

Copy link
Member Author

Choose a reason for hiding this comment

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

Please @lalitb @ThomsonTan @maxgolov review that PR :)

lalitb
lalitb approved these changes Jan 29, 2021
View reviewed changes
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM other than the suggested change.

@bogdandrutu bogdandrutu force-pushed the runformat branch 2 times, most recently from e082a7d to 1544016 Compare January 29, 2021 19:34
@bogdandrutu bogdandrutu changed the title Upgrade clang-format to 11.x, run ./tools/format.sh Upgrade clang-format to 10.0.0, run ./tools/format.sh Jan 29, 2021
@maxgolov
Copy link
Contributor

One minor thing we may want to check if Visual Studio 2019 has the same formatter. Right now it does respect the .clang-format file, but it'd be a minor nuisance to disable the IDE formatter (or configure the new one manually) in case if it is 9 instead of 10.

@maxgolov
Copy link
Contributor

We're good. I verified that vs2019 does have 10 as the default in the latest update.

@bogdandrutu
Copy link
Member Author

@maxgolov what is that? Are you not using vim? :))

@lalitb lalitb merged commit 20db113 into open-telemetry:main Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lalitb lalitb lalitb approved these changes

@ThomsonTan ThomsonTan ThomsonTan left review comments

@maxgolov maxgolov maxgolov approved these changes

@jsuereth jsuereth jsuereth 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.
5 participants
@bogdandrutu @maxgolov @jsuereth @ThomsonTan @lalitb