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

Provide codesigned stub exe's #5252

Merged
merged 7 commits into from
Apr 29, 2024
Merged

Provide codesigned stub exe's #5252

merged 7 commits into from
Apr 29, 2024

Conversation

Callek
Copy link
Contributor

@Callek Callek commented Mar 23, 2024 •
edited
Loading

Description

  • This provides codesigned exe's for conda's stubs
  • Relates to conda/conda #13685
  • Signatures provided on behalf of Anaconda

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Mar 23, 2024
@Callek Callek force-pushed the callek/sign-stubs branch from ae43ff3 to 6ef531c Compare March 23, 2024 02:55
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Mar 23, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #5252 will improve performances by ×9.8

Comparing Callek:callek/sign-stubs (e9aeb03) with main (ab137d2)

Summary

⚡ 2 improvements
✅ 1 untouched benchmarks

Benchmarks breakdown

Benchmark main Callek:callek/sign-stubs Change
⚡ test_pin_subpackage_benchmark 122.8 s 70.6 s +73.93%
⚡ test_select_lines_battery 1,108.3 ms 112.9 ms ×9.8

@Callek Callek marked this pull request as ready for review March 23, 2024 04:16
@Callek Callek requested a review from a team as a code owner March 23, 2024 04:16
@Callek Callek force-pushed the callek/sign-stubs branch from 6374cd3 to 6710c45 Compare March 25, 2024 13:37
beeankha
beeankha reviewed Mar 25, 2024
View reviewed changes
news/5252-sign-stubs Outdated Show resolved Hide resolved
Callek and others added 3 commits March 25, 2024 10:24
@Callek @schuylermartin45
Update tests/test_codesigned.py
f7c18e0 
Co-authored-by: Schuyler Martin <schuylermartin45@gmail.com>
@Callek @beeankha
Update news/5252-sign-stubs
e152952 
Co-authored-by: Bianca Henderson <beeankha@gmail.com>
@Callek
pre-commit
edb51e8
schuylermartin45
schuylermartin45 reviewed Mar 25, 2024
View reviewed changes
tests/test_codesigned.py

from conda_build.utils import on_win

HERE = os.path.abspath(os.path.dirname(__file__))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be mixing pathlib and os.path calls in the same file? Or do we want to be consistent in usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no Pathlib variant of shutil.which and other tests don't seem to use Pathlib much at first glance, but I'm a big proponent of Pathlib.

Copy link
Member

Choose a reason for hiding this comment

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

We've been slowly moving paths in conda over to pathlib, but rest assured it backfired in messy code where paths are handed around and assumed strings.

schuylermartin45
schuylermartin45 previously approved these changes Mar 25, 2024
View reviewed changes
Copy link
Contributor

@schuylermartin45 schuylermartin45 left a comment

Choose a reason for hiding this comment

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

Some nit-picky comments but overall I'm glad to see additional testing.

I think the logic for detecting cases where the test can't run looks correct!

@Callek
Copy link
Contributor Author

Callek commented Mar 25, 2024

Some nit-picky comments but overall I'm glad to see additional testing.

Spent like 5x as long writing the tests as I did signing the files, but decided we didn't want to be surprised in the future when these are no longer signed.

jezdez
jezdez reviewed Mar 25, 2024
View reviewed changes
Copy link
Member

@jezdez jezdez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not loving that we're adding the same code paths in conda in conda/conda#13721 as well as here. Let's discuss what you're trying to do with these tests. Verify that signtool works?

Also, like conda/conda#13721 this doesn't elaborate how the files were created that we're shipping after this is merged, which is a big miss and should be recorded in this pull request for posterity. Frankly I'm a little concerned that the blobs aren't created in a transparent process via a CD system.

tests/test_codesigned.py

from conda_build.utils import on_win

HERE = os.path.abspath(os.path.dirname(__file__))
Copy link
Member

Choose a reason for hiding this comment

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

We've been slowly moving paths in conda over to pathlib, but rest assured it backfired in messy code where paths are handed around and assumed strings.

@jezdez jezdez requested review from chenghlee and kenodegard April 15, 2024 18:35
@jezdez
Copy link
Member

jezdez commented Apr 15, 2024

@chenghlee Please take a look at these (which are less urgent than the linked feature in conda) but would still benefit from your seal of approval.

@Callek Callek changed the title Add test for checking the stubs are signed Provide codesigned stub exe's Apr 16, 2024
beeankha
beeankha previously approved these changes Apr 16, 2024
View reviewed changes
jezdez
jezdez previously approved these changes Apr 24, 2024
View reviewed changes
jezdez
jezdez reviewed Apr 24, 2024
View reviewed changes
news/5252-sign-stubs Outdated Show resolved Hide resolved
@jezdez jezdez dismissed stale reviews from beeankha and schuylermartin45 via e9aeb03 April 26, 2024 18:40
@jezdez jezdez merged commit 3e9fe71 into conda:main Apr 29, 2024
28 checks passed
@jezdez jezdez mentioned this pull request Apr 29, 2024
Closed
2 tasks
@jezdez jezdez added this to the 24.5.x milestone Apr 29, 2024
@jaimergp jaimergp mentioned this pull request Aug 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenghlee chenghlee chenghlee approved these changes

@jezdez jezdez jezdez approved these changes

@beeankha beeankha beeankha left review comments

@schuylermartin45 schuylermartin45 schuylermartin45 left review comments

@kenodegard kenodegard Awaiting requested review from kenodegard

Assignees
No one assigned
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
🔎 Review
Archived in project
Milestone
24.5.x
Development

Successfully merging this pull request may close these issues.
6 participants
@Callek @jezdez @schuylermartin45 @chenghlee @beeankha @conda-bot