-
Notifications
You must be signed in to change notification settings - Fork 45
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
verify swig version in setup.py #811
Conversation
setup.py
Outdated
swig_version_line = proc.stdout.read().split('\n')[1] | ||
swig_version = swig_version_line.split(' ')[2] | ||
if not swig_version.startswith('3'): |
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 guess as #360 mentions swig 4 breaks Agg backend specifically. Maybe this should just be a warning here instead of raising? Saying beware agg won't work but if you wanna use non agg, go right ahead as you are?
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.
Maybe this should just be a warning here instead of raising?
I think this should be an error. Warnings during installation might be ignored.
@@ -106,7 +106,6 @@ | |||
"traitsui", | |||
"pyface", | |||
"pypdf2", | |||
"swig", |
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'm not sure why you removed swig
from the dependencies list here. Without this, swig
won't get installed via edm into the python environment.
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.
it was listed twice, see line 104, I should have mentioned that
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.
ahh. i should have noticed that things were not in alphabetical order - which i what i was kinda expecting.
can you rearrange the set alphabetically, for readability? this could be done in a later PR
setup.py
Outdated
"is currently a known issue with SWIG 4.0, see enthought/enable#360." | ||
" Please download SWIG 3.0.x (see http://www.swig.org/).") | ||
try: | ||
with subprocess.Popen(["swig", "-version"], stdout=subprocess.PIPE, encoding='utf-8') as proc: |
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.
it looks like we don't run flake8 on ci.
setup.py
Outdated
swig_version_line = proc.stdout.read().split('\n')[1] | ||
swig_version = swig_version_line.split(' ')[2] | ||
if not swig_version.startswith('3'): |
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.
Maybe this should just be a warning here instead of raising?
I think this should be an error. Warnings during installation might be ignored.
swig_version = swig_version_line.split(' ')[2] | ||
if not swig_version.startswith('3'): | ||
raise Exception(msg) | ||
except FileNotFoundError: |
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'm not entirely sure what raises the FileNotFoundError
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.
if swig
is not installed, then swig -version
causes this:
$ brew uninstall swig
Uninstalling /usr/local/Cellar/swig/4.0.2... (725 files, 5.4MB)
(dumb)~/Desktop/enable/dist aayres (verify-swig-version) $ swig -version
bash: /usr/local/bin/swig: No such file or directory
(dumb)~/Desktop/enable/dist aayres (verify-swig-version) $ pip install enable-5.1.0.dev287.tar.gz
Processing ./enable-5.1.0.dev287.tar.gz
Installing build dependencies ... done
Getting requirements to build wheel ... error
ERROR: Command errored out with exit status 1:
command: /Users/aayres/.edm/envs/dumb/bin/python3 /Users/aayres/.edm/envs/dumb/lib/python3.6/site-packages/pip/_vendor/pep517/_in_process.py get_requires_for_build_wheel /var/folders/j9/ps8s26f91_1_lgf7mtd4smyr0000gn/T/tmp5st4ag4b
cwd: /private/var/folders/j9/ps8s26f91_1_lgf7mtd4smyr0000gn/T/pip-req-build-w6n7h3n2
Complete output (29 lines):
Traceback (most recent call last):
File "setup.py", line 439, in verify_swig_version
stdout=subprocess.PIPE, encoding='utf-8') as proc:
File "/Users/aayres/.edm/envs/dumb/lib/python3.6/subprocess.py", line 729, in __init__
restore_signals, start_new_session)
File "/Users/aayres/.edm/envs/dumb/lib/python3.6/subprocess.py", line 1364, in _execute_child
raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'swig': 'swig'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/aayres/.edm/envs/dumb/lib/python3.6/site-packages/pip/_vendor/pep517/_in_process.py", line 257, in <module>
main()
File "/Users/aayres/.edm/envs/dumb/lib/python3.6/site-packages/pip/_vendor/pep517/_in_process.py", line 240, in main
json_out['return_val'] = hook(**hook_input['kwargs'])
File "/Users/aayres/.edm/envs/dumb/lib/python3.6/site-packages/pip/_vendor/pep517/_in_process.py", line 91, in get_requires_for_build_wheel
return hook(config_settings)
File "/private/var/folders/j9/ps8s26f91_1_lgf7mtd4smyr0000gn/T/pip-build-env-017ig1uv/overlay/lib/python3.6/site-packages/setuptools/build_meta.py", line 155, in get_requires_for_build_wheel
config_settings, requirements=['wheel'])
File "/private/var/folders/j9/ps8s26f91_1_lgf7mtd4smyr0000gn/T/pip-build-env-017ig1uv/overlay/lib/python3.6/site-packages/setuptools/build_meta.py", line 135, in _get_build_requires
self.run_setup()
File "/private/var/folders/j9/ps8s26f91_1_lgf7mtd4smyr0000gn/T/pip-build-env-017ig1uv/overlay/lib/python3.6/site-packages/setuptools/build_meta.py", line 150, in run_setup
exec(compile(code, __file__, 'exec'), locals())
File "setup.py", line 449, in <module>
verify_swig_version()
File "setup.py", line 446, in verify_swig_version
raise Exception(msg)
Exception: SWIG is a required build dependency of Enable. Furthermore, there is currently a known issue with SWIG 4.0, see enthought/enable#360. Please install SWIG 3.0.x (see http://www.swig.org/).
----------------------------------------
ERROR: Command errored out with exit status 1: /Users/aayres/.edm/envs/dumb/bin/python3 /Users/aayres/.edm/envs/dumb/lib/python3.6/site-packages/pip/_vendor/pep517/_in_process.py get_requires_for_build_wheel /var/folders/j9/ps8s26f91_1_lgf7mtd4smyr0000gn/T/tmp5st4ag4b Check the logs for full command output.
Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>
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.
@@ -106,7 +106,6 @@ | |||
"traitsui", | |||
"pyface", | |||
"pypdf2", | |||
"swig", |
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.
ahh. i should have noticed that things were not in alphabetical order - which i what i was kinda expecting.
can you rearrange the set alphabetically, for readability? this could be done in a later PR
Do we not run flake8 as part of CI in this project? |
We don't from the looks of it. |
This is a follow up to #808 which closes #769
This PR checks the version of swig via a subprocess call (a little ugly but seems to get the job done) in setutp.py.
If swig is not around or we have a bad version (not version 3.x) we raise an exception. This follows the second point at the end of the 769 issue description