search

textile / python-textile

A Python port of Textile, A humane web text generator
Other
68 stars 23 forks source link

Missing files in the tarball #33

Closed thmo closed 7 years ago

thmo commented 7 years ago

Please include

README.textile
LICENSE.txt

in the tarball. For building the Fedora package, we have to separately download them.

It might also be good to have the tests dir as part of the tarball, so we can run the testsuite while creating the RPM.

sebix commented 7 years ago

Regarding the files: True, and also the changelog and contributors files are missing in the pip-package. Not sure about the todo.

It might also be good to have the tests dir as part of the tarball, so we can run the testsuite while creating the RPM.

We could move it to textile and make it a submodule, would this be ok for you?

thmo commented 7 years ago

Not sure about the todo.

That's probably not needed. We usually don't include it in the "binary" RPM.

It might also be good to have the tests dir as part of the tarball, so we can run the testsuite while creating the RPM.

We could move it to textile and make it a submodule, would this be ok for you?

On this I am a bit undecided. http://doc.pytest.org/en/latest/goodpractices.html#choosing-a-test-layout-import-rules shows both layouts (inline and extra dir). From an RPM packager pov, I'd say the tests should not be installed system-wide (as they are intended for developers), so I'd favor the extra dir layout (which you currently use). I am not sure however, how to trigger its inclusion for sdist in setup.py, maybe via MANIFEST.in listing tests, pytest.ini, etc.

sebix commented 7 years ago

You could always do a rm -r %{buildroot}%{python3_sitelib}/textile/tests/ in your specfile :D

Please test it: https://testpypi.python.org/pypi/textile/2.3.3 The tests/ dir is included in the source tarball, but not in the wheel. If it's correct, I will push the used MANIFEST.in.

I'm not sure if we should do a new release because of the last 3 fixes, which all affected only the packaging. But I can't upload to the real pypi anyway.

thmo commented 7 years ago

Please test it: https://testpypi.python.org/pypi/textile/2.3.3 The tests/ dir is included in the source tarball, but not in the wheel. If it's correct, I will push the used MANIFEST.in.

Looks good to me.

Two questions:

As a side note, .coveragerc is also not in the tarball (and might itself be updated, as it has omit = textile/tests/* in it.)

ikirudennis commented 7 years ago

@sebix please submit a pull request for those changes. It sounds like they should be trivial to merge in and make a release for them. Thanks

@thmo Yes, please create a separate issue for those failures. And the pytest.ini specifies the --cov argument as a default option. It's still Sunday morning here, and I'm not sure my brain is at full speed yet: Is pytest.ini being excluded from the sdist? It might need to be included along with .coveragerc.

sebix commented 7 years ago

Okay, I'll include the .coveragerc too.

thmo commented 7 years ago

@ikirudennis For some reason I currently don't understand, the failures don't show up anymore.

@sebix This tarball does neither contain a pytest.ini nor a .coveragerc.

sebix commented 7 years ago

@thmo Fixed with a new file: https://testpypi.python.org/pypi/textile/ (the 2.3.3-1)

ikirudennis commented 7 years ago

@thmo, we've all been there before, right? If @sebix's file works for you, I'll merge it, and push it out to pypi.

thmo commented 7 years ago

The updated tarfile works for me, although it contains additional, different changes - for example it installs a /usr/bin/textile binary.

This binary is welcomed in principle, but - at least for Fedora - there's a name clash. /usr/bin/textile is already provided by the perl-Text-Textile package, so I'd have to rename it to something else. e.g. /usr/bin/pytextile. Suggestions?

sebix commented 7 years ago

This command has been added a while ago in 0be616bdb9ef93ca4c744b58fc303512b8c5de1d

concerning the clash: What about providing e.g. /usr/bin/py(thon-)textile(-3.5) as default and /usr/bin/textile via update-alternatives?

thmo commented 7 years ago

This command has been added a while ago in 0be616b

That's on branch develop, and has not been merged to master. Thus it's also not present in 2.3.3.

sebix commented 7 years ago

That's on branch develop, and has not been merged to master. Thus it's also not present in 2.3.3.

Yes, but develop is the next release. Thus it will be in 2.3.4

ikirudennis commented 7 years ago

I had a feeling some sort of name collision would happen with the command line tool. I'm fine with changing it to pytextile. Would we need to change it in the setup.py, or are you able to change it as @sebix asked? (I'm not terribly familiar with packaging for Fedora, so please forgive my ignorance.)

thmo commented 7 years ago

If you change it in the setup.py, it's slightly easier for me, but more importantly, there's a chance that the name will be the same across distros. One of the Fedora principles is to get as much as possible upstream.

That said, renaming it as part of the packaging would also be no big deal.

Adding the update-alternatives machinery is a bit more work and would require coordination with the packager of the perl package.

sebix commented 7 years ago

Fixed in fa20408

mitya57 commented 7 years ago

textile-2.3.5.tar.gz still does not include tests, despite what the changelog says.

ikirudennis commented 7 years ago

I just uploaded version 2.3.6 which now has the tests directory.

mitya57 commented 7 years ago

Ok, thanks, will be useful for me in Debian.

sebix commented 7 years ago

9998e8ea3a0d33ced9d0afbb6406d29231bb98f6 added the tests-directory, available in release 2.3.6. But it's not in in 2.3.7 for some reasons.

ikirudennis commented 7 years ago

Hmm, this is strange. I'm not sure why, but setup.py is being weird. I ran it as is, and it didn't include it. I had to change it to include tests/* and it included it. I reverted the change, removed the dist directory, and re-ran it and it was included. So, I'll remember to keep an eye on it whenever I create a new release. Thanks for bringing it to my attention.

sebix commented 7 years ago

How do you create the source tarballs? Running python setup.py sdist locally includes the tests/fixtures, while the archive on pypi does not have it. But the archive there has a lot of temporary files such as tests/.test_github_issues.py.un~.

sebix commented 7 years ago

And where do the files textile/rawlink_textile.py and textile/plaintext.py come from?

ikirudennis commented 7 years ago

Yeah, I run python setup.py sdist bdist_wheel. Building both of them simultaneously shouldn't affect either build, right? I'll do some experimenting.

That's weird about rawlink and plaintext. I had no idea they would be included in the zip.

Thanks for the eagle eyes on this. I'll see about cleaning it up and re-upload.

thmo commented 7 years ago

Hm, these two files now ended up in the 2.3.8 Fedora RPMs. Hope this doesn't create confusion for someone...

Please do not upload a modfiied zip file with the same name, that's bad style. Either add some postfix to, or simply increase the version number.

sebix commented 7 years ago

Hm, these two files now ended up in the 2.3.8 Fedora RPMs. Hope this doesn't create confusion for someone...

I also noticed it during packaging (for openSUSE) because rpmlint complained about the shebangs :) But the existence of these files shouldn't cause problems.

thmo commented 7 years ago

I also noticed it during packaging (for openSUSE) because rpmlint complained about the shebangs :)

In the Fedora spec file, I always remove the shebangs from all .py files anyway, so I didn't notice ;)

sebix commented 7 years ago

In 2.3.9 the two files are gone now, but tests/fixtures/README.txt is still missing.

ikirudennis commented 7 years ago

I gotta say, this whole MANIFEST and sdist thing has me pretty baffled. It's not consistent across machines or even consecutive runs on the same machine.

I'll see what I can cook up.

ikirudennis commented 7 years ago

So, I decided to start from scratch with MANIFEST.in. And it turns out without it, the packaging is pretty smart without a manifest. It just needed an entry for test fixture, and the manifest file itself in order to include them in the source distribution. The icing on the cake is that it turns out that those rawlink and plaintext files which were mistakenly included in the source, have actually been included in the wheels of previous versions as well. It's looking like 2.3.10 will be a bit slimmer than previous versions.

Thanks again, @sebix and @thmo for your attention to detail.