Skip to content

Revert Python finding logic and some CMake fixes for Windows #6382

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
merged 4 commits into from
Sep 25, 2024
Merged

Revert Python finding logic and some CMake fixes for Windows #6382

merged 4 commits into from
Sep 25, 2024

Conversation

cyyever
Copy link
Contributor

@cyyever cyyever commented Sep 22, 2024 •
edited
Loading

Description

Revert the python finding logic introduced by #6381 due to unable to handle Conda builds. However, the Python executable detection was improved. In addition, some Windows warnings were re-enabled, and the hidden visibility of onnx_cpp2py_export was fixed.

Motivation and Context

For better code.

@cyyever
Re-enable w4251 on Windows and disable w4146
d262e5e 
Signed-off-by: cyy <cyyever@outlook.com>
@cyyever cyyever requested a review from a team as a code owner September 22, 2024 00:51
@cyyever cyyever marked this pull request as draft September 22, 2024 00:51
@codecov Codecov
Copy link

codecov bot commented Sep 22, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.23%. Comparing base (b196423) to head (2a999c2).
Report is 340 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #6382   +/-   ##
=======================================
  Coverage   57.23%   57.23%           
=======================================
  Files         507      507           
  Lines       31406    31406           
  Branches     4690     4690           
=======================================
  Hits        17975    17975           
  Misses      12576    12576           
  Partials      855      855

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

@cyyever
Re-enable w4996 on Windows
08ab56c 
Signed-off-by: cyy <cyyever@outlook.com>
@cyyever cyyever changed the title Re-enable w4251 on Windows and disable w4146 Re-enable w4251, on Windows and disable w4146 Sep 22, 2024
@cyyever cyyever changed the title Re-enable w4251, on Windows and disable w4146 Re-enable w4251,w4996 and disable w4146 on Windows Sep 22, 2024
@cyyever cyyever changed the title Re-enable w4251,w4996 and disable w4146 on Windows Refine warnings on Windows and other CMake fixes Sep 22, 2024
@cyyever cyyever marked this pull request as ready for review September 22, 2024 01:54
@cyyever cyyever force-pushed the onnx_win branch 2 times, most recently from f23305a to 694bfa9 Compare September 22, 2024 02:28
@cyyever cyyever marked this pull request as draft September 22, 2024 02:30
@cyyever cyyever force-pushed the onnx_win branch 3 times, most recently from f8596f7 to 1d17001 Compare September 22, 2024 03:14
@cyyever cyyever marked this pull request as ready for review September 22, 2024 03:37
@cyyever cyyever changed the title Refine warnings on Windows and other CMake fixes Some CMake fixes for Windows Sep 22, 2024
@justinchuby
Copy link
Member

Could you explain in PR description what errors were fixed?

@cyyever cyyever requested a review from justinchuby September 22, 2024 04:48
@cyyever
Copy link
Contributor Author

cyyever commented Sep 22, 2024

More details were added to the PR description.

@andife andife added the run release CIs Use this label to trigger release tests in CI label Sep 22, 2024
@justinchuby
Copy link
Member

Could NOT find Python (missing: Python_INCLUDE_DIRS Python_LIBRARIES
Interpreter Development Development.Module Development.Embed)
Call Stack (most recent call first):
/tmp/build-env-qekth2v4/lib/python3.8/site-packages/cmake/data/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:603 (_FPHSA_FAILURE_MESSAGE)
/tmp/build-env-qekth2v4/lib/python3.8/site-packages/cmake/data/share/cmake-3.30/Modules/FindPython.cmake:672 (find_package_handle_standard_args)
CMakeLists.txt:108 (find_package)

@justinchuby
Copy link
Member

https://github.com/onnx/onnx/actions/runs/10978690342/job/30482133230?pr=6382

@cyyever
Copy link
Contributor Author

cyyever commented Sep 23, 2024 •
edited
Loading

@justinchuby Should be fixed by passing Python_INCLUDE_DIRS from setup.py to CMake. Feel free to trigger more CI tests.

@andife andife added run release CIs Use this label to trigger release tests in CI and removed run release CIs Use this label to trigger release tests in CI labels Sep 23, 2024
@justinchuby justinchuby added run release CIs Use this label to trigger release tests in CI and removed run release CIs Use this label to trigger release tests in CI labels Sep 24, 2024
@andife
Copy link
Member

andife commented Sep 24, 2024

@andife is it possible for the workflows to be triggered by pull requests, once the label is present?

I think I have a variant of how this could be implemented, I am currently not aware of any out-of-box variant, so I would have to delve deeper into the documentation.

@justinchuby
Copy link
Member

@andife is it possible for the workflows to be triggered by pull requests, once the label is present?

I think I have a variant of how this could be implemented, I am currently not aware of any out-of-box variant, so I would have to delve deeper into the documentation.

Would something like trigger by pull request, and if the trigger is PR && label is not present, skip the workflow work?

@cyyever cyyever force-pushed the onnx_win branch 4 times, most recently from 4868b8e to fe8bfb2 Compare September 24, 2024 06:05
@cyyever cyyever marked this pull request as draft September 24, 2024 06:12
@cyyever
Revert to old Python finding logic and improve the detection of Pytho…
2a999c2 
…n executable

Signed-off-by: cyy <cyyever@outlook.com>
@cyyever cyyever marked this pull request as ready for review September 24, 2024 06:49
@cyyever cyyever changed the title Some CMake fixes for Windows Improved Python finding logic and some CMake fixes for Windows Sep 24, 2024
@cyyever cyyever changed the title Improved Python finding logic and some CMake fixes for Windows Revert Python finding logic and some CMake fixes for Windows Sep 24, 2024
@justinchuby justinchuby added run release CIs Use this label to trigger release tests in CI and removed run release CIs Use this label to trigger release tests in CI labels Sep 24, 2024
justinchuby
justinchuby reviewed Sep 24, 2024
View reviewed changes
justinchuby
justinchuby approved these changes Sep 25, 2024
View reviewed changes
Copy link
Member

@justinchuby justinchuby left a comment

Choose a reason for hiding this comment

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

Thanks!

@justinchuby justinchuby added this pull request to the merge queue Sep 25, 2024
Merged via the queue into onnx:main with commit d14634e Sep 25, 2024
73 checks passed
@cyyever cyyever deleted the onnx_win branch September 25, 2024 02:14
linshokaku pushed a commit to linshokaku/onnx that referenced this pull request Oct 2, 2024
@cyyever @linshokaku
Revert Python finding logic and some CMake fixes for Windows (onnx#6382)
f3b5f05 
### Description
<!-- - Describe your changes. -->
Revert the python finding logic introduced by onnx#6381 due to unable to
handle Conda builds. However, the Python executable detection was
improved. In addition, some Windows warnings were re-enabled, and the
hidden visibility of onnx_cpp2py_export was fixed.
### Motivation and Context
For better code.
<!-- - Why is this change required? What problem does it solve? -->
<!-- - If it fixes an open issue, please link to the issue here. -->

---------

Signed-off-by: cyy <cyyever@outlook.com>
Signed-off-by: Linsho Kaku <linsho@preferred.jp>
@ramkrishna2910 ramkrishna2910 added the topic: build Issues related to ONNX builds and packages label May 12, 2025
@botantony botantony mentioned this pull request May 14, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinchuby justinchuby justinchuby approved these changes

Assignees
No one assigned
Labels
run release CIs Use this label to trigger release tests in CI topic: build Issues related to ONNX builds and packages
Projects
PR Tracker
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cyyever @justinchuby @andife @ramkrishna2910