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

Use setuptools instead of distutils for Python > 3.8. #2113

Merged
merged 2 commits into from
Jul 30, 2023
Merged

Use setuptools instead of distutils for Python > 3.8. #2113

merged 2 commits into from
Jul 30, 2023

Conversation

ni4
Copy link
Contributor

@ni4 ni4 commented Jul 26, 2023

Fixes #2112

@ni4 ni4 mentioned this pull request Jul 26, 2023
Closed
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 26, 2023
View reviewed changes
src/tests/cli_common.py Fixed Show fixed Hide fixed
@remicollet
Copy link
Contributor

I confirm this patch works as expected

@ni4 ni4 force-pushed the ni4-2112-fix-deprecated-distutils branch from 0807fc9 to c3de464 Compare July 26, 2023 10:17
@ni4
Copy link
Contributor Author

ni4 commented Jul 26, 2023

@remicollet Thanks for confirming. Wondering how that worked without find_executable. Please see the updated patch.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 26, 2023
View reviewed changes
src/tests/cli_common.py Fixed Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 26, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (8e17480) 83.88% compared to head (14a49b5) 83.88%.

❗ Current head 14a49b5 differs from pull request most recent head 6967bcb. Consider uploading reports for the commit 6967bcb to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2113   +/-   ##
=======================================
  Coverage   83.88%   83.88%           
=======================================
  Files         161      161           
  Lines       32249    32249           
=======================================
  Hits        27052    27052           
  Misses       5197     5197

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

remicollet
remicollet reviewed Jul 26, 2023
View reviewed changes
src/tests/cli_common.py
@@ -78,7 +82,7 @@ def file_text(path, encoding = CONSOLE_ENCODING):
return f.read().decode(encoding).replace('\r\r', '\r')

def find_utility(name, exitifnone = True):
path = distutils.spawn.find_executable(name)
path = shutil.which(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not tested, but setuptools._distutils.spawn provides distutils.spawn and find_executable.

# head /usr/lib/python3.11/site-packages/setuptools/_distutils/spawn.py
"""distutils.spawn

Provides the 'spawn()' function, a front-end to various platform-
specific functions for launching another program in a sub-process.
Also provides the 'find_executable()' to search the path for a given
executable name.
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Let's use shutil.which as it seems to be easier solution. Or do you know any disadvantages?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes shutil.which seems the simpler.

LGTM

@remicollet
Copy link
Contributor

Tested with various python versions (Fedora, RHEL-8 and 9) using shlib (and removing import of distutils)
Build OK.

@ronaldtse
Copy link
Contributor

@ni4 other than the comments from @remicollet there is also a failure in Cirrus:

https://cirrus-ci.com/task/6093471186944000

The following tests FAILED:
	258 - cli_tests-Encryption (Failed)
Errors while running CTest

@ni4 ni4 force-pushed the ni4-2112-fix-deprecated-distutils branch from c3de464 to 76e627c Compare July 27, 2023 13:26
@ni4
Copy link
Contributor Author

ni4 commented Jul 27, 2023

@ni4 other than the comments from @remicollet there is also a failure in Cirrus:

https://cirrus-ci.com/task/6093471186944000

The following tests FAILED:
	258 - cli_tests-Encryption (Failed)
Errors while running CTest

Thanks, re-pushed. cli_tests-Encryption occasionally fails due to GnuPG's handling of multiple SKESKs.

@ni4 ni4 force-pushed the ni4-2112-fix-deprecated-distutils branch from 06f9d9b to 6967bcb Compare July 28, 2023 15:00
@ronaldtse
Copy link
Contributor

Ping @antonsviridenko for approval. Thanks!

@ni4
Copy link
Contributor Author

ni4 commented Jul 29, 2023

@maxirmx Do you have an idea why Windows-2019 OpenSSL runners fail? I added some debug logging, but those doesn't given any idea to me.

@maxirmx
Copy link
Member

maxirmx commented Jul 29, 2023

@ni4
It looks like there is some critical issue in between vcpkg and nasm: https://github.com/rnpgp/rnp/actions/runs/5700501434

It does not look like the issue you mentioned but we have to wait until it is resolved

@antonsviridenko
Copy link
Contributor 

 -- Could NOT find OpenSSL, try to set the path to OpenSSL root folder in the system variable OPENSSL_ROOT_DIR (missing: OPENSSL_CRYPTO_LIBRARY OPENSSL_INCLUDE_DIR) 
CMake Error at C:/Program Files/CMake/share/cmake-3.27/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find OpenSSL, try to set the path to OpenSSL root folder in the
  system variable OPENSSL_ROOT_DIR (missing: OPENSSL_CRYPTO_LIBRARY
  OPENSSL_INCLUDE_DIR) (Required is at least version "1.1.1")
Call Stack (most recent call first):
-- Configuring incomplete, errors occurred!
  C:/Program Files/CMake/share/cmake-3.27/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  C:/Program Files/CMake/share/cmake-3.27/Modules/FindOpenSSL.cmake:668 (find_package_handle_standard_args)
  src/lib/CMakeLists.txt:44 (find_package)

antonsviridenko
antonsviridenko approved these changes Jul 29, 2023
View reviewed changes
Copy link
Contributor

@antonsviridenko antonsviridenko 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.

@ronaldtse I'm not an expert in Python, but otherwise looks good.

@maxirmx
Copy link
Member

maxirmx commented Jul 29, 2023

microsoft/vcpkg#32600
Our scripts were operating from hash but hash has been updated in line with vcpkg
So now Windows/openssl is totally broken

  • I suggest to disable MSVC/openssl build for now
  • If this issue persists we can consider other openssl Windows distribution. Shining Light or something similar

@ni4
Copy link
Contributor Author

ni4 commented Jul 30, 2023

@maxirmx Thanks for the investigation!
@ronaldtse Should we merge this as is, given that patch doesn't have nothing to deal with OpenSSL/Windows?

@ronaldtse
Copy link
Contributor

Agree @ni4 , let’s merge first. Thanks!

@ronaldtse ronaldtse merged commit 4d84f4e into main Jul 30, 2023
109 of 111 checks passed
@ronaldtse ronaldtse deleted the ni4-2112-fix-deprecated-distutils branch July 30, 2023 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remicollet remicollet remicollet left review comments

@antonsviridenko antonsviridenko antonsviridenko approved these changes

@ronaldtse ronaldtse ronaldtse approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build failure with python3.12
5 participants
@ni4 @remicollet @ronaldtse @maxirmx @antonsviridenko