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

Update clang to upstream chromium m114's clang-17 #2680

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

niranjanyardi
Copy link
Contributor

b/323184809

@niranjanyardi niranjanyardi changed the base branch from clang17_linux to main March 22, 2024 04:02
@niranjanyardi niranjanyardi force-pushed the clang17_linux branch 2 times, most recently from b77b65b to 9e5dbac Compare March 22, 2024 04:17
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.86%. Comparing base (94df067) to head (c3c5032).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2680      +/-   ##
==========================================
+ Coverage   58.82%   58.86%   +0.03%     
==========================================
  Files        1906     1906              
  Lines       93605    93605              
==========================================
+ Hits        55060    55096      +36     
+ Misses      38545    38509      -36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@kaidokert kaidokert left a comment

Choose a reason for hiding this comment

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

Lets clean up the variable names so we don't have churn later.

I'd also consider reducing some duplication maybe, e.g. move the install code into a shared .sh file between the two containers, but that's up to you. It's more of a separate refactoring problem.

@niranjanyardi
Copy link
Contributor Author

Lets clean up the variable names so we don't have churn later.

I'd also consider reducing some duplication maybe, e.g. move the install code into a shared .sh file between the two containers, but that's up to you. It's more of a separate refactoring problem.

done, will carry out the refactor in a follow up

Copy link
Member

@kaidokert kaidokert left a comment

Choose a reason for hiding this comment

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

Looks okay - please be very careful while landing this, as it impacts a lot of containers.

@niranjanyardi
Copy link
Contributor Author

Looks okay - please be very careful while landing this, as it impacts a lot of containers.

agreed, my plan is to test this out on internal builds before pushing with @briantting 's help.

@niranjanyardi niranjanyardi force-pushed the clang17_linux branch 2 times, most recently from c7186e4 to 2646ab9 Compare March 25, 2024 22:10
Change-Id: I5faeaae796d0b6d50e590d1267c3512a265a9f9a
Change-Id: I1038f7d435660ad39036b177f375ed3d9529562b
@niranjanyardi niranjanyardi merged commit 92151b6 into youtube:main Mar 27, 2024
382 of 384 checks passed
niranjanyardi added a commit that referenced this pull request Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants