Skip to content

Fix #11089 - Relax codesigning entitlements for Python on mac #11090

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
Merged
Myoldmopar merged 4 commits into from
Jun 2, 2025
Merged

Fix #11089 - Relax codesigning entitlements for Python on mac #11090

Myoldmopar merged 4 commits into from
Jun 2, 2025

Conversation

@jmarrec
Copy link
Contributor

@jmarrec jmarrec commented Jun 2, 2025

Pull request overview

Description of the purpose of this PR

Sign energyplus CLI with com.apple.security.cs.disable-library-validation so that we can load python native extensions that aren't signed with E+ certificate.

Pull Request Author

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

jmarrec added 3 commits June 2, 2025 10:19
@jmarrec
Add DEEP and ENTITLEMENTS option to codesign_macos + register_install…
0b397e1 
…_codesign_target takes a COMPONENT argument
@jmarrec
register_install_codesign_target takes an optional ENTITLEMENTS singl…
ee7057d 
…eValueKeyword now, sign E+ with it

Without the com.apple.security.cs.disable-library-validation I can't later on pip install C-extensions into the python_lib/ folder. This is done on OpenStudio SDK in particular...
@jmarrec
proper fix for the codesigning of the fortran utilities and other cma…
4d2e94e 
…ke project()
@jmarrec jmarrec requested a review from Myoldmopar June 2, 2025 10:53
@jmarrec jmarrec self-assigned this Jun 2, 2025
@jmarrec jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Jun 2, 2025
@jmarrec
Copy link
Contributor Author

jmarrec commented Jun 2, 2025 •
edited
Loading

I have tested this PR at https://github.com/jmarrec/EnergyPlus/releases/tag/v25.1.0-WithDSOASpaceListFixes-python-entitlements and confirm that I can now run the defect file I provided.

jmarrec
jmarrec commented Jun 2, 2025
View reviewed changes
cmake/python_entitlements.xml
Comment on lines +1 to +9
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<!-- This is needed because otherwise you can't pip install stuff into the python_lib folder -->
<key>com.apple.security.cs.disable-library-validation</key>
<true/>
</dict>
</plist>
Copy link
Contributor Author

@jmarrec jmarrec Jun 2, 2025

Choose a reason for hiding this comment

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

entitlements

src/EnergyPlus/CMakeLists.txt
Comment on lines +1230 to +1231
register_install_codesign_target(energyplus "." Unspecified ENTITLEMENTS "${CMAKE_CURRENT_SOURCE_DIR}/../../cmake/python_entitlements.xml")
register_install_codesign_target(energyplusapi "." Unspecified)
Copy link
Contributor Author

@jmarrec jmarrec Jun 2, 2025

Choose a reason for hiding this comment

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

And use it.

The rest of this PR is just improvements around the cmake scripts, but produces no difference.

Copy link
Contributor Author

@jmarrec jmarrec Jun 2, 2025

Choose a reason for hiding this comment

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

Here is the before/after output of running ninja package &> ori_log.txt (or new_log.txt)

https://gist.github.com/jmarrec/ddfab0530b97b6acc0af3832bdb434da

.github/workflows/test_code_integrity.yml
Comment on lines +43 to +44
readarray -t changed_files < <(git diff --name-only HEAD^ HEAD src/ tst/ | /bin/grep -E '\.(cpp|cc|c|hpp|hh|h)$')
file_count=${#changed_files[@]}
Copy link
Contributor Author

@jmarrec jmarrec Jun 2, 2025

Choose a reason for hiding this comment

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

Avoid the clang-format tripping on the case where you do have changed files but none that are C/C++. I've seen @Myoldmopar mention it on another PR recently, just won't bother looking for the link right now.

Copy link
Member

@Myoldmopar Myoldmopar Jun 2, 2025

Choose a reason for hiding this comment

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

Yeah, this is good. I don't think I've used readarray for a decade. The -t to trim off each newline is a great fix for this.

Myoldmopar
Myoldmopar approved these changes Jun 2, 2025
View reviewed changes
Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

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

Looks good to me.

.github/workflows/test_code_integrity.yml
Comment on lines +43 to +44
readarray -t changed_files < <(git diff --name-only HEAD^ HEAD src/ tst/ | /bin/grep -E '\.(cpp|cc|c|hpp|hh|h)$')
file_count=${#changed_files[@]}
Copy link
Member

@Myoldmopar Myoldmopar Jun 2, 2025

Choose a reason for hiding this comment

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

Yeah, this is good. I don't think I've used readarray for a decade. The -t to trim off each newline is a great fix for this.

@Myoldmopar
Copy link
Member

Myoldmopar commented Jun 2, 2025

Alright this can go in to clean up those issues. Thanks @jmarrec

@Myoldmopar Myoldmopar merged commit e909b50 into develop Jun 2, 2025
9 checks passed
@Myoldmopar Myoldmopar deleted the 11089_python_entitlements branch June 2, 2025 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Myoldmopar Myoldmopar Myoldmopar approved these changes

Assignees

@jmarrec jmarrec

Labels

Defect Includes code to repair a defect in EnergyPlus

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Relax codesigning entitlements for Python on mac

5 participants

@jmarrec @Myoldmopar @nrel-bot-2 @nrel-bot-2c