-
Notifications
You must be signed in to change notification settings - Fork 112
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
Build script improvements #221
base: master
Are you sure you want to change the base?
Conversation
build_wheels.py
Outdated
@@ -1,23 +1,14 @@ | |||
import os |
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.
Since this is a script, it should probably be executable and have a shebang?
build_wheels.py
Outdated
pass | ||
|
||
for platform, archs in architectures.items(): | ||
def make_wheel(platform, arch, dist): |
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.
The name make_wheel
is not quite accurate, since it's also used to make sdist
.
build_wheels.py
Outdated
|
||
for platform, archs in architectures.items(): | ||
def make_wheel(platform, arch, dist): | ||
os.system('python setup.py clean --all') |
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.
os.system()
is a bit old-school and could probably be replaced with subprocess.run()
https://docs.python.org/3/library/subprocess.html#subprocess.run.
This would also provide a nice way to set the environment with env
.
build_wheels.py
Outdated
|
||
for platform, archs in architectures.items(): | ||
def make_wheel(platform, arch, dist): | ||
os.system('python setup.py clean --all') |
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.
Simply using 'python'
uses Python 2.7 on my (Debian Linux) system (which isn't really a problem but it was probably not intended).
You should probably use sys.executable
instead.
setup.py
Outdated
exec(line) | ||
break | ||
else: | ||
raise RuntimeException('No version number found') |
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.
That's not a thing.
build_wheels.py
Outdated
make_wheel('darwin', '64bit', 'bdist_wheel') | ||
make_wheel('win32', '32bit', 'bdist_wheel') | ||
make_wheel('win32', '64bit', 'bdist_wheel') | ||
make_wheel('', '', 'bdist_wheel') |
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.
Before running sdist
, you should remove the egg-info
directory, as I've done in https://github.com/spatialaudio/python-sounddevice/blob/master/make_dist.sh.
Cool, this has made the build script a bit cleaner. I've used it to compile 0.10.2 and it seems to work great! |
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.
Thanks, it indeed looks quite streamlined.
@@ -1,23 +1,20 @@ | |||
import os | |||
#!/usr/bin/env python |
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.
subprocess.run()
is a Python 3 feature, so this should probably be python3
?
In fact, you created a tag I don't see the "sdist" in https://pypi.org/project/SoundFile/#files, but probably that's just because I don't know my way around the new warehouse? What are the plans for the deprecated package https://pypi.org/project/PySoundFile/? |
BTW, the wheels don't seem to support PyPy 3.4 and 3.5. Also CPython 3.7 is coming up in June ... |
Haha, that's a funny typo! All kidding aside, we could start thinking about a 1.0.0.
Actually, I forgot to upload that. Thanks for pointing that out. I'll have to create a separate upload script. |
I would advise against that in the current situation.
I don't know if that's really necessary, since AFAIK the currently recommended way to upload stuff is a one-liner anyway:
How did you upload? |
Referring back to #220, we need to improve our build process.
Right now, we should be building correct cross-platform wheels.
However, the source distribution currently includes all binaries, which is not intended.
Also, there is currently no deprecation notice for PySoundFile.