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.

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

setup.py: add "wheel" to setup_requires #349

Closed
wants to merge 32 commits into from
Closed

setup.py: add "wheel" to setup_requires #349

wants to merge 32 commits into from

Conversation

alanvgreen
Copy link
Contributor

Fixes problem when installing into a python venv created with

$ python3 -m venv my_env

See m-labs/nmigen#323

Fixes problem when installing into a python venv created with

$ python3 -m venv my_env

See m-labs/nmigen#323
@whitequark
Copy link
Member

Do you know why this dependency is necessary? It looks like Python is trying to build a wheel here, but the wheel library doesn't exist, which seems like a strange situation.

Does this only happen if you install nmigen from git in virtualenv, and not otherwise?

@codecov
Copy link

codecov bot commented Apr 10, 2020

Codecov Report

Merging #349 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #349   +/-   ##
=======================================
  Coverage   82.64%   82.64%           
=======================================
  Files          35       35           
  Lines        5948     5948           
  Branches     1208     1208           
=======================================
  Hits         4916     4916           
  Misses        868      868           
  Partials      164      164           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3899b2b...3899b2b. Read the comment docs.

whitequark and others added 13 commits April 12, 2020 03:28
The default __repr__() from typing.NamedTuple does not include
the module name, so the replacement (which uses the preferred syntax
for specifying these shapes) doesn't either.
Before this commit, selecting a part that was fully out of bounds of
a value was correctly implemented as a write to a dummy wire, but
selecting a part that was only partially out of bounds resulted in
a crash.

Fixes #351.
Such wires are likely to trigger pathological behavior in Yosys and,
if applicable, other toolchains that consume Verilog converted from
RTLIL.

Fixes #341.
By default, if an operation produces an undefined value (a Jinja2
concept that corresponds to Python's KeyError, AttributeError, etc)
then this value may be printed in a template, which is a nop. This
behavior can hide bugs.

This commit changes the Jinja2 behavior to raise an error instead of
producing an undefined value in all cases. (We produce undefined
values deliberately in a few places. Those are unaffected; it is OK
to use several kinds of undefined values in one Jinja2 environment.)

Fixes #337.
@FFY00
Copy link
Contributor

FFY00 commented Apr 14, 2020

This does not belong in setup_requires, at most, it would be in extras_require. Although, I can't reproduce, it seems like a bug in your setup.

@whitequark
Copy link
Member

I am not able to reproduce either:

~$ virtualenv venvtest
Using base prefix '/usr'
New python executable in /home/whitequark/venvtest/bin/python3
Also creating executable in /home/whitequark/venvtest/bin/python
Installing setuptools, pip, wheel...
done.
~$ cd venvtest/
~/venvtest$ . bin/activate
(venvtest) ~/venvtest$ pip install git+https://github.com/nmigen/nmigen.git
Collecting git+https://github.com/nmigen/nmigen.git
  Cloning https://github.com/nmigen/nmigen.git to /tmp/pip-req-build-7qdlu_3v
  Running command git clone -q https://github.com/nmigen/nmigen.git /tmp/pip-req-build-7qdlu_3v
Requirement already satisfied: setuptools in ./lib/python3.7/site-packages (from nmigen==0.3.dev33+g3346f2c) (46.1.3)
Collecting pyvcd~=0.2.0
  Downloading pyvcd-0.2.1-py2.py3-none-any.whl (17 kB)
Collecting Jinja2~=2.11
  Downloading Jinja2-2.11.2-py2.py3-none-any.whl (125 kB)
     |████████████████████████████████| 125 kB 2.7 MB/s
Collecting MarkupSafe>=0.23
  Downloading MarkupSafe-1.1.1-cp37-cp37m-manylinux1_x86_64.whl (27 kB)
Building wheels for collected packages: nmigen
  Building wheel for nmigen (setup.py) ... done
  Created wheel for nmigen: filename=nmigen-0.3.dev33+g3346f2c-py3-none-any.whl size=174944 sha256=50252426adffbfbd67f6d9ce85df534d9e1ea16c2872252e56f2e62fb4452bd7
  Stored in directory: /tmp/pip-ephem-wheel-cache-yzut3x95/wheels/82/71/f3/45c024858a8a85e66b358edd4a555c975f96a4e343a1d21e90
Successfully built nmigen
Installing collected packages: pyvcd, MarkupSafe, Jinja2, nmigen
Successfully installed Jinja2-2.11.2 MarkupSafe-1.1.1 nmigen-0.3.dev33+g3346f2c pyvcd-0.2.1
(venvtest) ~/venvtest$

@mithro
Copy link

mithro commented Apr 14, 2020

I think this is a difference between virtualenv and venv (which are confusingly different tools). venv doesn't install wheel into the environment by default it seems while virtualenv does. I think virtualenv --no-wheel would give you the same behaviour as venv.

@FFY00
Copy link
Contributor

FFY00 commented Apr 14, 2020

I created the environment with venv, just as described and I was able to install nmigen. Maybe an older version of venv didn't install wheel?

Either way, there is nothing in nmigen that explicitly requires wheel. If setuptools_scm is using wheel for the installation then wheel is its dependency. If wheel was an optional dependency of setuptools_scm and was required here, then it should be in setup_requires, but it isn't.

@mithro
Copy link

mithro commented Apr 14, 2020

@FFY00 - Your on Arch Linux right? I think the problem is with how Ubuntu / Debianism mangles Python packages. Is wheel is an optional dependency of setuptools / pip?

@FFY00
Copy link
Contributor

FFY00 commented Apr 14, 2020

Then it would be a Debian bug. No, wheel is not a dependency for either setuptool_scm or pip, at least in the latest versions.

whitequark and others added 7 commits April 15, 2020 01:31
Before this commit, selecting a part that was fully out of bounds of
a value was correctly implemented as a write to a dummy wire, but
selecting a part that was only partially out of bounds resulted in
a crash.

Fixes #351.
Such wires are likely to trigger pathological behavior in Yosys and,
if applicable, other toolchains that consume Verilog converted from
RTLIL.

Fixes #341.
By default, if an operation produces an undefined value (a Jinja2
concept that corresponds to Python's KeyError, AttributeError, etc)
then this value may be printed in a template, which is a nop. This
behavior can hide bugs.

This commit changes the Jinja2 behavior to raise an error instead of
producing an undefined value in all cases. (We produce undefined
values deliberately in a few places. Those are unaffected; it is OK
to use several kinds of undefined values in one Jinja2 environment.)

Fixes #337.
Fixes problem when installing into a python venv created with

$ python3 -m venv my_env

See m-labs/nmigen#323
@alanvgreen
Copy link
Contributor Author

I am seeing the problem on Ubuntu.

Installation is successful with "wheel" in extra_requires rather than setup_requires.

One aspect of this problem is that wheel appears to be installed by default with virtualenv, but not with venv.

@FFY00
Copy link
Contributor

FFY00 commented Apr 15, 2020

How does your setup.py look like? There's is no extra_requires, it is called extras_require and it's not a list like setup_requires.

Can you post the exact commands to reproduce?

It seems like you messed up the history by doing a merge. Can you look at reflog, do a hard reset to a previous state and then rebase on master? This should result in a clean git tree with just one commit on top of master. Let me know if you need help with the commands.

@ohsix
Copy link

ohsix commented Apr 15, 2020

what version of ubuntu and python3?

@whitequark
Copy link
Member

@alanvgreen I believe you are hitting an issue with the way Debian ships Python packages. If you install python3-wheel from apt, do you still have this problem?

@alanvgreen
Copy link
Contributor Author

@ohsix : from the bug report

(life) osboxes@osboxes:~/life$ uname -a
Linux osboxes 5.3.0-45-generic #37-Ubuntu SMP Thu Mar 26 20:41:27 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
(life) osboxes@osboxes:~/life$ python --version
Python 3.7.5
(life) osboxes@osboxes:~/life$ lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 19.10
Release:	19.10
Codename:	eoan

Fixes problem when installing into a python venv created with

$ python3 -m venv my_env

See m-labs/nmigen#323
Fixes problem when installing into a python venv created with

$ python3 -m venv my_env

See m-labs/nmigen#323
@alanvgreen
Copy link
Contributor Author

@FFY00 Thanks for your kind offer, I think I managed to straighten out my pull request. Let me know if not.

Here is a repro:

osboxes@osboxes:~$ python3 -m venv test
osboxes@osboxes:~$ cd test
osboxes@osboxes:~/test$ . bin/activate
(test) osboxes@osboxes:~/test$ pip install nmigen
Collecting nmigen
  Downloading https://files.pythonhosted.org/packages/43/08/162d253c8d41460cffda0ca4d60f7fb075bb2b787b82089189c76a33f121/nmigen-0.2.tar.gz (144kB)
    100% |████████████████████████████████| 153kB 2.8MB/s 
Collecting Jinja2 (from nmigen)
  Using cached https://files.pythonhosted.org/packages/30/9e/f663a2aa66a09d838042ae1a2c5659828bb9b41ea3a6efa20a20fd92b121/Jinja2-2.11.2-py2.py3-none-any.whl
Collecting pyvcd~=0.1.4 (from nmigen)
  Using cached https://files.pythonhosted.org/packages/01/3e/2ff169806a094365722f6670cc777f83bd93c56471ccc260cc5e0ba36b50/pyvcd-0.1.7-py2.py3-none-any.whl
Requirement already satisfied: setuptools in ./lib/python3.7/site-packages (from nmigen) (40.8.0)
Collecting MarkupSafe>=0.23 (from Jinja2->nmigen)
  Using cached https://files.pythonhosted.org/packages/98/7b/ff284bd8c80654e471b769062a9b43cc5d03e7a615048d96f4619df8d420/MarkupSafe-1.1.1-cp37-cp37m-manylinux1_x86_64.whl
Collecting six (from pyvcd~=0.1.4->nmigen)
  Using cached https://files.pythonhosted.org/packages/65/eb/1f97cb97bfc2390a276969c6fae16075da282f5058082d4cb10c6c5c1dba/six-1.14.0-py2.py3-none-any.whl
Building wheels for collected packages: nmigen
  Running setup.py bdist_wheel for nmigen ... error
  Complete output from command /home/osboxes/test/bin/python3 -u -c "import setuptools, tokenize;__file__='/tmp/pip-install-p6ju8i6j/nmigen/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" bdist_wheel -d /tmp/pip-wheel-oeglfmeo --python-tag cp37:
  usage: -c [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
     or: -c --help [cmd1 cmd2 ...]
     or: -c --help-commands
     or: -c cmd --help
  
  error: invalid command 'bdist_wheel'
  
  ----------------------------------------
  Failed building wheel for nmigen
  Running setup.py clean for nmigen
Failed to build nmigen
Installing collected packages: MarkupSafe, Jinja2, six, pyvcd, nmigen
  Running setup.py install for nmigen ... done
Successfully installed Jinja2-2.11.2 MarkupSafe-1.1.1 nmigen-0.2 pyvcd-0.1.7 six-1.14.0
(test) osboxes@osboxes:~/test$ 

If I then attempt to pip install nmigen a second time, things look okay:

(test) osboxes@osboxes:~/test$ pip install nmigen
Requirement already satisfied: nmigen in ./lib/python3.7/site-packages (0.2)
Requirement already satisfied: Jinja2 in ./lib/python3.7/site-packages (from nmigen) (2.11.2)
Requirement already satisfied: pyvcd~=0.1.4 in ./lib/python3.7/site-packages (from nmigen) (0.1.7)
Requirement already satisfied: setuptools in ./lib/python3.7/site-packages (from nmigen) (40.8.0)
Requirement already satisfied: MarkupSafe>=0.23 in ./lib/python3.7/site-packages (from Jinja2->nmigen) (1.1.1)
Requirement already satisfied: six in ./lib/python3.7/site-packages (from pyvcd~=0.1.4->nmigen) (1.14.0)
(test) osboxes@osboxes:~/test$ python
Python 3.7.5 (default, Nov 20 2019, 09:21:52) 
[GCC 9.2.1 20191008] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from nmigen import *
>>> 

I don't know what to make of that - I wouldn't trust the installation.

Looking at https://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-extras-optional-features-with-their-own-dependencies I'm not sure why extras_require is the right thing to use here. What did you have in mind?

@alanvgreen
Copy link
Contributor Author

@whitequark It turns out that I already had python3-wheel installed. I can't recall whether I explicitly installed it before - it may have been a dependency of something else, but very likely it was something I tried while working through my problems.

At this stage, I'm quite happy for the resolution of this PR to be "use virtualenv, not venv" :)

@whitequark
Copy link
Member

It looks like the documentation for venv describes it as deprecated since 3.6. Is that right?

@programmerjake
Copy link
Contributor

It looks like the documentation for venv describes it as deprecated since 3.6. Is that right?

only the pyvenv script was deprecated where python -m venv is the recommended replacement:
https://docs.python.org/dev/whatsnew/3.6.html#id8

@whitequark
Copy link
Member

@alanvgreen If you're happy using virtualenv then I suggest closing this, since it seems very hard to reproduce. If someone manages to find a good solution here I'm happy to merge it as well.

@alanvgreen
Copy link
Contributor Author

Yup. Closing, thanks all!

@alanvgreen alanvgreen closed this Apr 15, 2020
@graingert
Copy link

graingert commented Apr 15, 2020

@alanvgreen try

pip install git+git://github.com/graingert/nmigen@4c6070d#egg=nmigen

@alanvgreen
Copy link
Contributor Author

@graingert Yes, that seems to work for me:

osboxes@osboxes:~$ python3 -m venv test
osboxes@osboxes:~$ cd test
osboxes@osboxes:~/test$ . bin/activate
(test) osboxes@osboxes:~/test$ pip install git+git://github.com/graingert/nmigen@4c6070d#egg=nmigen
Collecting nmigen from git+git://github.com/graingert/nmigen@4c6070d#egg=nmigen
  Cloning git://github.com/graingert/nmigen (to revision 4c6070d) to /tmp/pip-install-bzsob8tr/nmigen
  Did not find branch or tag '4c6070d', assuming revision or ref.
  Installing build dependencies ... done
Collecting pyvcd~=0.2.0 (from nmigen)
  Using cached https://files.pythonhosted.org/packages/95/44/1e4302b02fd5a5c65e2a3846b0a9468c7e3c9d52ba546d6ff1d17fa378a8/pyvcd-0.2.1-py2.py3-none-any.whl
Collecting Jinja2~=2.11 (from nmigen)
  Using cached https://files.pythonhosted.org/packages/30/9e/f663a2aa66a09d838042ae1a2c5659828bb9b41ea3a6efa20a20fd92b121/Jinja2-2.11.2-py2.py3-none-any.whl
Collecting MarkupSafe>=0.23 (from Jinja2~=2.11->nmigen)
  Using cached https://files.pythonhosted.org/packages/98/7b/ff284bd8c80654e471b769062a9b43cc5d03e7a615048d96f4619df8d420/MarkupSafe-1.1.1-cp37-cp37m-manylinux1_x86_64.whl
Building wheels for collected packages: nmigen
  Running setup.py bdist_wheel for nmigen ... done
  Stored in directory: /tmp/pip-ephem-wheel-cache-ekr8zajb/wheels/7f/7f/14/1d74905917927f926962a87dc38dafdb57ececd753331b2d89
Successfully built nmigen
Installing collected packages: pyvcd, MarkupSafe, Jinja2, nmigen
Successfully installed Jinja2-2.11.2 MarkupSafe-1.1.1 nmigen-0.3.dev36+g4c6070d pyvcd-0.2.1
(test) osboxes@osboxes:~/test$ 

@whitequark
Copy link
Member

@alanvgreen Thanks for confirming! There are some issues with #358 that prevent merging it in the short term, but I'll make sure that this is fixed at some point. For now, please use virtualenv.

whitequark added a commit that referenced this pull request Jul 1, 2020
This is necessary to be able to install nMigen into a virtualenv that
does not have `wheel` installed in certain cases.

See #349.
whitequark added a commit that referenced this pull request Jul 1, 2020
This is necessary to be able to install nMigen into a virtualenv that
does not have `wheel` installed in certain cases.

See #349.
@whitequark
Copy link
Member

@alanvgreen I fixed this properly in 7fca037.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants