Don't create pyc files when building tables #135

stefanor · 2016-07-27T23:50:32Z

We don't want to include pyc files in source distributions.

eliben · 2016-07-30T12:17:38Z

I'm not sure I follow. Shoudln't they just be excluded from the manifest?

It seems useful to create .pycs when doing install? and install calls _run_build_tables too

stefanor · 2016-07-30T20:05:21Z

Shouldn't they just be excluded from the manifest?

So, that was my initial thought too. But they are excluded from the manifest, yet still appearing in the sdist.

The problem here is that we are running a python script directly in the package build directory, between the source copying (that reads MANIFEST.in) and building the tarball.

It seems useful to create .pycs when doing install?

pip will do that on the actual install. Shipping pycs in the source tarballs is not the way to solve that problem.

and install calls _run_build_tables too

Yes 100%. This doesn't change that.

eliben · 2016-08-01T04:12:21Z

It seems useful to create .pycs when doing install?
pip will do that on the actual install.

Please bear with me here; there were a lot of issues with these tables in the past. The problem is, if there's no .pyc in the install directory, the .pyc is created locally where users of pycparser live, which is very bad because pycparser is used transitively but a lot of tools today.

Currently _build_tables is run during installation to generate new .py files that weren't shipped. I'm not 100% sure pip actually does that, since this is a custom step that happens after the actual installation.

Therefore, at the very least I wouldn't run with -B during install, perhaps only during sdist.

eliben · 2016-10-06T13:59:29Z

Any updates on this?

stefanor · 2016-12-30T14:37:48Z

Sorry, I haven't been on top of GitHub notifications.

there were a lot of issues with these tables in the past

My issue is entirely with the pyc files, which result from the way the tables are built. Not with the tables.

After that, I'm going to reply out of order, because I think that makes a little more sense.

Currently _build_tables is run during installation to generate new .py files that weren't shipped. I'm not 100% sure pip actually does that, since this is a custom step that happens after the actual installation.

Yeah, you can't do anything during installation in a wheel world, everything should be shipped in the wheel. However, you are running _build_tables during sdist which means those .py files are shipped in source tarballs. And running them during install which gets them into wheels. I think that is all correct. What I'm questioning is including .pyc files, which is a side-effect of the way _build_tables is being run.

The problem is, if there's no .pyc in the install directory, the .pyc is created locally where users of pycparser live

Are you sure about that? That sounds completely wrong. .pyc files should only be created next to their .py source, in Python 2, or in the neighbouring __pycache__ directory in Python 3. If the user doesn't have permission to write the .pyc files in those places, they simply don't get written, and Python doesn't complain. It does affect import performance, but that's it.

Therefore, at the very least I wouldn't run with -B during install, perhaps only during sdist.

I think that'd be wrong. Then you'd ship .pyc files in wheels. But the wheels target all versions of Python, and the .pyc files don't.

eliben · 2017-01-03T13:19:12Z

I think that'd be wrong. Then you'd ship .pyc files in wheels. But the wheels target all versions of Python, and the .pyc files don't.

If -B is used during sdist then there should be no .pyc files in the wheels, right?

bespokebob · 2018-04-18T20:06:51Z

If -B is used during sdist then there should be no .pyc files in the wheels, right?

bdist_wheel calls install, so if your install command is creating .pyc files, then they will also be included in the wheel.
See https://github.com/pypa/wheel/blob/master/wheel/bdist_wheel.py#L238

bespokebob · 2018-04-18T20:08:34Z

It is also completely wrong to include .pyc files in the sdist, as the .pyc files are being created using whatever version of Python that you used to create the sdist - not the version that the user is installing with.

eliben · 2018-04-24T12:24:24Z

#251 points here

If anyone wants to provide a well-tested PR that removes .pyc from the manifest and be responsibe for its testing and stability, I'll be happy to work together on this.

josephmancuso · 2018-04-24T12:35:37Z

For what it's worth there was a release around Jan 30th of this year with changes to .pyc files for Python 3.7 (something with a new hash based pyc)

josephmancuso · 2018-04-24T12:37:08Z

Like I said in my other comment on that issue referenced. I used the same version and it suddenly started breaking one day on the python 3.7 dev branch so I'm pretty sure it's a change in the dev branch around that time. ~Jan 24th

eliben · 2018-04-24T13:21:24Z

Fair enough.

I'm going to leave this open until a volunteer is found to fix the issue(s) with .pyc distribution once and for all. This is a messy aspect of Python, and I'm not personally interested in it for this project because I always use pycparser "from source" on the master git branch without actually installing anything. If distributing .pyc files is not best practice, so be it, but be mindful that this leads to other issues like those .pyc files being regenerated in the wrong places when users actually run scripts using the library (or worse, through several layers of dependencies). All of this has to be thoroughly tested and someone has to be ready and willing to intercept breakages and fix them - I'll be happy to release 2.19 that addresses this issue.

stefanor · 2018-04-30T22:06:59Z

FWIW, I tested this at the time :)

stefanor · 2018-04-30T22:21:53Z

Tested master + this branch:

Distribution	.py	.pyc	Utils	Tests	Tables	Comments
sdist	✔️	❌	✔️	✔️	❌	setup.pyc is included if present
bdist_wheel	✔️	❌	❌	❌	✔️	Universal py2+3
bdist	✔️	✔️	❌	❌	✔️
bdist_egg	✔️	✔️	❌	❌	❌	Missing tables

Those are all correct, I think, except for the egg (which nobody uses any more), and the setup.pyc accident.

eliben · 2018-05-01T02:10:24Z

Can you explain the difference between the 2nd and 3rd lines?

stefanor · 2018-05-01T15:14:07Z

bdist is an ancient thing. I hope nobody actually uses them. bdists are targetted at a specific python version on a specific platform, so there is no harm in containing pyc files.

AFAIK wheels never contain pyc files.

stefanor · 2018-05-01T16:01:08Z

I think setup.pyc is coming from include setup.* in MANIFEST.in, we can fix that...

Edit: Actually, without the setup.*, I was still getting it, but an exclude seems to do the trick.

eliben · 2018-05-01T19:34:35Z

MANIFEST.in

@@ -6,6 +6,7 @@ include README.*
 include LICENSE
 include CHANGES
 include setup.*


Should this just say include setup.py now?

I tried that, and it still included the pyc. I didn't go too far down that rabbit hole.

I mean have "include setup.py" and "exclude setup.pyc". The "*" seems mysterious now

There's also setup.cfg

eliben · 2018-05-01T19:35:10Z

setup.py

-    call([sys.executable, '_build_tables.py'],
-         cwd=os.path.join(dir, 'pycparser'))
+    from subprocess import check_call
+    check_call([sys.executable, '-B', '_build_tables.py'],


Would be good to add a comment here explaining why the -B and even linking to the PR discussion

eliben · 2018-05-01T20:21:46Z

setup.py

-    call([sys.executable, '_build_tables.py'],
-         cwd=os.path.join(dir, 'pycparser'))
+    from subprocess import check_call
+    # This is run inside the install dir, so we don't want to leave any .pyc


It's not clear what the install dir means here - the directory you install it from, or install it to? Please be more explicit

eliben · 2018-05-02T18:20:15Z

Merging, thanks

stefanor added 2 commits July 27, 2016 16:48

Don't create pyc files when building tables

1fe4f52

We don't want to include pyc files in source distributions.

Don't ignore failures building tables

fc9d992

eliben added the pending-user-input label Oct 10, 2016

eliben closed this Jul 4, 2017

mgeier mentioned this pull request Apr 12, 2018

Installation on Python 3.7 raises ValueError: bad marshal data (unknown type code) #251

Closed

eliben added the patches-welcome label Apr 24, 2018

eliben reopened this Apr 24, 2018

Don't include setup.pyc

f13fc65

stefanor force-pushed the no-pyc branch from b5bea1d to f13fc65 Compare May 1, 2018 16:13

eliben requested changes May 1, 2018

View reviewed changes

eliben reviewed May 1, 2018

View reviewed changes

Explain the -B in a comment

6d91cbb

stefanor force-pushed the no-pyc branch from 4c0b041 to 6d91cbb Compare May 1, 2018 20:41

eliben merged commit aedb4ed into eliben:master May 2, 2018

stefanor deleted the no-pyc branch May 2, 2018 22:33

brucealthompson mentioned this pull request May 25, 2019

<python-pycparser>: build error openwrt/packages#9066

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't create pyc files when building tables #135

Don't create pyc files when building tables #135

stefanor commented Jul 27, 2016

eliben commented Jul 30, 2016

stefanor commented Jul 30, 2016 •

edited

Loading

eliben commented Aug 1, 2016

eliben commented Oct 6, 2016

stefanor commented Dec 30, 2016

eliben commented Jan 3, 2017

bespokebob commented Apr 18, 2018

bespokebob commented Apr 18, 2018

eliben commented Apr 24, 2018

josephmancuso commented Apr 24, 2018 •

edited

Loading

josephmancuso commented Apr 24, 2018 •

edited

Loading

eliben commented Apr 24, 2018

stefanor commented Apr 30, 2018

stefanor commented Apr 30, 2018

eliben commented May 1, 2018

stefanor commented May 1, 2018

stefanor commented May 1, 2018 •

edited

Loading

eliben May 1, 2018

stefanor May 1, 2018

eliben May 1, 2018

stefanor May 1, 2018

eliben May 1, 2018

eliben May 1, 2018

stefanor May 2, 2018

eliben commented May 2, 2018

Don't create pyc files when building tables #135

Don't create pyc files when building tables #135

Conversation

stefanor commented Jul 27, 2016

eliben commented Jul 30, 2016

stefanor commented Jul 30, 2016 • edited Loading

eliben commented Aug 1, 2016

eliben commented Oct 6, 2016

stefanor commented Dec 30, 2016

eliben commented Jan 3, 2017

bespokebob commented Apr 18, 2018

bespokebob commented Apr 18, 2018

eliben commented Apr 24, 2018

josephmancuso commented Apr 24, 2018 • edited Loading

josephmancuso commented Apr 24, 2018 • edited Loading

eliben commented Apr 24, 2018

stefanor commented Apr 30, 2018

stefanor commented Apr 30, 2018

eliben commented May 1, 2018

stefanor commented May 1, 2018

stefanor commented May 1, 2018 • edited Loading

eliben May 1, 2018

Choose a reason for hiding this comment

stefanor May 1, 2018

Choose a reason for hiding this comment

eliben May 1, 2018

Choose a reason for hiding this comment

stefanor May 1, 2018

Choose a reason for hiding this comment

eliben May 1, 2018

Choose a reason for hiding this comment

eliben May 1, 2018

Choose a reason for hiding this comment

stefanor May 2, 2018

Choose a reason for hiding this comment

eliben commented May 2, 2018

stefanor commented Jul 30, 2016 •

edited

Loading

josephmancuso commented Apr 24, 2018 •

edited

Loading

josephmancuso commented Apr 24, 2018 •

edited

Loading

stefanor commented May 1, 2018 •

edited

Loading