-
Notifications
You must be signed in to change notification settings - Fork 103
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
actions: add step for checking miral and miroil symbols map #3336
actions: add step for checking miral and miroil symbols map #3336
Conversation
cb3e542
to
57e1a4e
Compare
fc2ff06
to
3223157
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/symbols_generator #3336 +/- ##
=============================================================
+ Coverage 77.34% 77.35% +0.01%
=============================================================
Files 1062 1072 +10
Lines 67817 68216 +399
=============================================================
+ Hits 52453 52769 +316
- Misses 15364 15447 +83 ☔ View full report in Codecov by Sentry. |
8808130
to
4d3eba4
Compare
This reverts commit 7169146.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Approving, but not merging, to give the security question a little more airing.
.github/workflows/symbols-check.yml
Outdated
- name: Install clang | ||
run: | | ||
wget https://apt.llvm.org/llvm.sh | ||
chmod +x llvm.sh | ||
sudo ./llvm.sh 19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, this appears to work correctly with clang-19 from 24.04 (once that becomes available in GH, I guess)
.github/workflows/symbols-check.yml
Outdated
|
||
- name: Install clang | ||
run: | | ||
wget https://apt.llvm.org/llvm.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. What are the security concerns with this curl | sudo sh -
?
It's in the GitHub runners infrastructure, so infrastructure escape shouldn't be a problem, but what does it have access to here?
I think the answer is that it's got access to the GitHub secrets? Which means it has access to the release signing key, and the signing key to upload to the PPA?
I don't think this should be a blocker here, but maybe we should record this somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We changed this to at least add it from the repository
5d2e82f
to
6ba8032
Compare
6ba8032
to
0ab1bdb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits inline
…ing exit(1) Co-authored-by: Michał Sawicz <[email protected]>
|
||
- name: Install dependencies | ||
run: | | ||
sudo apt-get build-dep ./ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the expensive step :)
sudo apt-get build-dep ./ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want the build deps so that 3rd party libraries resolve in clang, right?
Co-authored-by: Michał Sawicz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
No description provided.