Packaging: Add placeholder packages #472

tiran · 2017-02-16T14:29:37Z

The ipa and freeipa packages are placeholders to prevent PyPI squashing
attacks and reserve the names for future use. pip install ipa installs
ipaclient.

Signed-off-by: Christian Heimes cheimes@redhat.com

The new PR provides just the two placeholder packages from PR #379.

MartinBasti · 2017-02-17T15:27:13Z

Shouldn't we have build dependency on python[3]-wheel without it bdist_wheel target is not working

MartinBasti · 2017-02-17T15:37:35Z

I'm not an expert about how PyPI is working, but shouldn't be there also placeholder packages for:

ipaserver
ipaplatform
ipatests

How about [free]ipa-server and friends, can a bad person use this for an attack when somebody uses pip install freeipa-server ?

MartinBasti · 2017-02-17T15:53:49Z

configure.ac

@@ -577,6 +577,9 @@ AC_CONFIG_FILES([
    ipaserver/Makefile
    ipatests/Makefile
    ipatests/man/Makefile
+    packaging/Makefile


can we rename this to wheel_packaging or pypi_packaging to be clear what is the purpose, because it is only for bdist_wheel

Sure! how about just pypi?

or pypi_placeholders

tiran · 2017-02-17T16:03:33Z

At the moment wheels are not required for RPM building. python-wheel is not available on RHEL, but I can work around it. Should the RPM spec file only contain dependencies for RPM packaging or also dependencies for general development and general packaging?

I can create placeholder modules for ipaserver, ipaplatform and ipatests, too. It's going to be a bit more tricky. Let's see...

HonzaCholasta · 2017-02-20T08:02:50Z

Is this really the right thing to do? IMO it does not make much sense to have placeholders for every ipa* package, as it does not scale at all - nothing is preventing a potential attacker to register their own ipa* package, which will confuse PyPI users all the same and will prevent us to use that name ourselves in the future, should we want to.

MartinBasti · 2017-02-20T11:33:08Z

We want to prevent others to have packages in PyPI with the same names as used for IPA. This is reasonable for protecting users to get attacker code from PyPI and rewrite working modules installed from rpms. In case that somebody install ipamodulefromhell we really cannot help this user

tiran · 2017-02-21T08:55:38Z

Yes, it is the right thing to do. You can trust in the expert with a decade of experience with Python packaging (formerly known as cheese shop).

MartinBasti · 2017-02-21T11:22:23Z

I talked with Honza how to handle the build dependency for pypi, and we may to remove the commit that adds python-wheel or add new option to specfile that will install pypi related packages with_pypi or so.

Do you plan to have more dependencies related only to pypi?

tiran · 2017-02-21T11:30:54Z

You requsted a dependency in the first place :)

If you are going to add a special build or dependency flavor for PyPI packaging, please also add python[23]-twine. It's the uploader tool we are going to use to upload packages to PyPI.

tiran · 2017-02-22T08:24:09Z

OK, you got with_wheels in freeipa.spec.in now. with_wheels is more logical than with_pypi because wheels have more uses than just PyPI upload.

MartinBasti · 2017-02-22T09:23:49Z

Thank you.

I see errors reported by pylint

************* Module ipaserver.install.installutils
ipaserver/install/installutils.py:1209: [E1101(no-member), store_version] Module 'ipaplatform' has no 'NAME' member)
ipaserver/install/installutils.py:1221: [E1101(no-member), check_version] Module 'ipaplatform' has no 'NAME' member)
ipaserver/install/installutils.py:1224: [E1101(no-member), check_version] Module 'ipaplatform' has no 'NAME' member)

MartinBasti · 2017-02-22T17:59:14Z

LGTM, please rebase and I will test it.

tiran · 2017-02-22T18:33:57Z

@MartinBasti I have rebased the branch and added wheel + placeholder building to make check. The pylint violations have disappeared.

MartinBasti · 2017-02-23T08:23:15Z

Is really needed to have this in make check? It makes build dependencies of wheel mandratory not optional

error: invalid command 'bdist_wheel'
make[5]: *** [bdist_wheel] Error 1
Makefile:591: recipe for target 'bdist_wheel' failed
make[5]: Leaving directory '/freeipa/rpmbuild/BUILD/freeipa-4.4.90.dev201702221837+git07cd377/pypi/freeipa'
Makefile:1192: recipe for target 'wheel_placeholder' failed
make[4]: *** [wheel_placeholder] Error 1
make[4]: *** Waiting for unfinished jobs....
usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
   or: setup.py --help [cmd1 cmd2 ...]
   or: setup.py --help-commands
   or: setup.py cmd --help

tiran · 2017-02-23T08:33:05Z

I'm open to suggestions here, but I like to have automatic validation of packaging.

lslebodn · 2017-02-23T08:40:26Z

I agree with @MartinBasti it is not a unit test.
IMHO better approach is to test it in CI/travis/...

tiran · 2017-02-23T08:46:43Z

That's not the point here. We are arguing about a new build dependency (python-wheel).

lslebodn · 2017-02-23T08:59:28Z

FYI: At the moment, make check run just C-based unit test and all of them are optional.
If required dependency is not found at configure time then test is not build/executed.

The reason is that required dependencies are not in some downstream distributions and IIRC python-wheel is not there either.

pvoborni · 2017-02-23T09:02:46Z

Some distros like RHEL doesn't have python-wheel packaged. It can be disabled by downstream patch, but better would be to remove it or make it configurable.

lslebodn · 2017-02-23T09:04:59Z

NACK for downstream patch. The intention of build system refactoring was make packaging in downstream simpler.

MartinBasti · 2017-02-23T09:41:40Z

@tiran I thought we agreed on having with_wheels in specfile and install dependencies only when you want to build wheel packages, what is not the case of RHEL. So what is the issue with python-wheel?

My only concern it to not run wheel build in make check

tiran · 2017-02-23T09:54:23Z

@MartinBasti I dropped the last commit. make check no longer checks wheel packages.

I'm going to open a new ticket for @martbab and ask him to add to add a proper test for wheel building to his awesome container magic.

MartinBasti · 2017-02-23T11:43:44Z

The old pylint issue is back

************* Module ipaserver.install.installutils
ipaserver/install/installutils.py:1209: [E1101(no-member), store_version] Module 'ipaplatform' has no 'NAME' member)
ipaserver/install/installutils.py:1221: [E1101(no-member), check_version] Module 'ipaplatform' has no 'NAME' member)
ipaserver/install/installutils.py:1224: [E1101(no-member), check_version] Module 'ipaplatform' has no 'NAME' member)

lslebodn · 2017-02-23T13:08:25Z

Makefile.am

+pypi_packages: bdist_wheel wheel_placeholder
+	@echo -e "\n\nTo upload packages to PyPI, run:\n"
+	@echo -e "    twine upload $(WHEELDISTDIR)/*-$(VERSION)-py2.py3-none-any.whl\n"
+


Is there a reason why top-level make need to be polluted with this new build targets?
They can be just in pypi/Makefile.am.
It will simplify IPA_PLACEHOLDERS; you will iterate over all SUBDIRS there.

Yes, there are. If you look closely you will notice that the new target depends on bdist_wheel and wheel_placeholder and the placeholder parts creates wheels in dist/wheels. It doesn't maek sense to have the target just in pypi.

My first version iterated over subdirs in ./pypi. This doesn't work for ipacommands (next PR) because ipacommands must be built as sdist, not bdist_wheel.

You can move all following make targets bdist_wheel wheel_bundle wheel_placeholder pypi_packages into pypi/Makefile.am
(including all dependencies.

The top level make file will be simpler. The same as before wheel/pypy effort.
And Makefile.am in sub directory pypy will be python only.

I respectfully decline your suggestion.

It's much more important to make package building and testing easy to use than to keep the top level Makefile simple.

The top level Makefile is the public interface. Sublevel makefiles are helpers. I don't like to have targets in sublevel makefiles that are not usable from the top level Makefile.

The targets are easily discoverable, .e.g. with make <tab>.

</bikeshedding>

I like to have bdist_wheel accessible from the top makefile too

that could be solved with

wheel_placeholder: $(MAKE) -C $(top_builddir)/pypi wheel_placeholder

It would make the main Makefile nicer+simpler but that's just nice to have

The main problem is not in this PR but in structure of directories in freeipa git.
There are not sub-directories for Python code/C-code/javascript code...
As a result of this there are many *.SUBDIR variables in top level makefile to workaround this directory structure.

I thought that moving some *.SUBDIR variables to pypi/Makefile.am would simplify it.

Sorry for a little noise.

tiran · 2017-02-23T13:18:09Z

Commit 1f8326a fixes an issue in Makefile.python.am. I think the issue caused ipaplatform and pypi/ipaplatform to cross streams.

The ipa and freeipa packages are placeholders to prevent PyPI squashing attacks and reserve the names for future use. `pip install ipa` installs ipaclient. Signed-off-by: Christian Heimes <cheimes@redhat.com>

Signed-off-by: Christian Heimes <cheimes@redhat.com>

I also renamed the base directory to pypi and added a new build target pypi_packages. Signed-off-by: Christian Heimes <cheimes@redhat.com>

Signed-off-by: Christian Heimes <cheimes@redhat.com>

Some calls to setup.py specified a build base, some did not. This can lead to issues, e.g. build, clean and install are using different build directories. Signed-off-by: Christian Heimes <cheimes@redhat.com>

pylint gets confused by duplicated package names, e.g. ipaplatform and pypi/ipaplatform. Signed-off-by: Christian Heimes <cheimes@redhat.com>

MartinBasti · 2017-02-23T16:35:14Z

Makefile.python.am

@@ -20,7 +19,7 @@ all-local: $(top_builddir)/ipasetup.py
 		    --build-base "$(abs_builddir)/build"

 install-exec-local: $(top_builddir)/ipasetup.py
-	if [ "x$(pkginstall)" = "xtrue" ]; then \
+	if [ "x$(pkginstall)" != "xfalse" ]; then \


I miss explanation why in commit message

Default to pkginstall=true without duplicated definitions

automake was complaining about duplicated definition. I dropped pkginstall=true from Makefile.python.am and only set it in special Makefile.am

automake was complaining about duplicated definitions of pkginstall. It was defined to true in Makefile.python.am only to be overriden in some Makefile.am. Now we assume that pkginstall is implicit true and only skip installation when pkginstall is explicitly set to false. Signed-off-by: Christian Heimes <cheimes@redhat.com>

MartinBasti · 2017-03-02T13:44:03Z

master:

2e78433 Packaging: Add placeholder packages
e2b9ea2 Add python-wheel as build requirement
acdd1f5 Add placeholders for ipaplatform, ipaserver and ipatests
b4c1bf1 Add with_wheels global to install wheel and PyPI packaging dependencies
ab9f42d Python build: use --build-base everywhere
60cfacc pylint: ignore pypi placeholders
bc1f60b Default to pkginstall=true without duplicated definitions

tiran requested a review from MartinBasti February 16, 2017 14:29

MartinBasti self-assigned this Feb 17, 2017

MartinBasti reviewed Feb 17, 2017

View reviewed changes

tiran force-pushed the placeholder_packages branch from 1218451 to 2963ad3 Compare February 17, 2017 16:47

tiran force-pushed the placeholder_packages branch from 2963ad3 to ce38000 Compare February 21, 2017 10:06

tiran force-pushed the placeholder_packages branch from ce38000 to 8b8a1dc Compare February 22, 2017 08:21

tiran force-pushed the placeholder_packages branch from 8b8a1dc to 1febac3 Compare February 22, 2017 18:32

tiran force-pushed the placeholder_packages branch from 1febac3 to 53f3d41 Compare February 23, 2017 09:52

lslebodn reviewed Feb 23, 2017

View reviewed changes

tiran added 5 commits February 23, 2017 15:14

Packaging: Add placeholder packages

41acd1d

The ipa and freeipa packages are placeholders to prevent PyPI squashing attacks and reserve the names for future use. `pip install ipa` installs ipaclient. Signed-off-by: Christian Heimes <cheimes@redhat.com>

Add python-wheel as build requirement

eabac68

Signed-off-by: Christian Heimes <cheimes@redhat.com>

Add placeholders for ipaplatform, ipaserver and ipatests

e269b89

I also renamed the base directory to pypi and added a new build target pypi_packages. Signed-off-by: Christian Heimes <cheimes@redhat.com>

Add with_wheels global to install wheel and PyPI packaging dependencies

69a606b

Signed-off-by: Christian Heimes <cheimes@redhat.com>

Python build: use --build-base everywhere

6839d95

Some calls to setup.py specified a build base, some did not. This can lead to issues, e.g. build, clean and install are using different build directories. Signed-off-by: Christian Heimes <cheimes@redhat.com>

tiran force-pushed the placeholder_packages branch from 1f8326a to 6839d95 Compare February 23, 2017 14:14

pylint: ignore pypi placeholders

1387012

pylint gets confused by duplicated package names, e.g. ipaplatform and pypi/ipaplatform. Signed-off-by: Christian Heimes <cheimes@redhat.com>

tiran mentioned this pull request Feb 23, 2017

Packaging: Add IPA commands package #379

Closed

MartinBasti suggested changes Feb 23, 2017

View reviewed changes

tiran force-pushed the placeholder_packages branch from b443513 to 15535d6 Compare February 23, 2017 17:32

MartinBasti approved these changes Mar 1, 2017

View reviewed changes

MartinBasti added the ack Pull Request approved, can be merged label Mar 1, 2017

MartinBasti added the pushed Pull Request has already been pushed label Mar 2, 2017

MartinBasti closed this Mar 2, 2017

tiran deleted the placeholder_packages branch March 15, 2017 10:35

Packaging: Add placeholder packages #472

Packaging: Add placeholder packages #472

tiran commented Feb 16, 2017 •

edited by MartinBasti

MartinBasti commented Feb 17, 2017

MartinBasti commented Feb 17, 2017

MartinBasti Feb 17, 2017

tiran Feb 17, 2017

MartinBasti Feb 17, 2017

tiran commented Feb 17, 2017

HonzaCholasta commented Feb 20, 2017

MartinBasti commented Feb 20, 2017

tiran commented Feb 21, 2017

MartinBasti commented Feb 21, 2017

tiran commented Feb 21, 2017

tiran commented Feb 22, 2017

MartinBasti commented Feb 22, 2017

MartinBasti commented Feb 22, 2017

tiran commented Feb 22, 2017

MartinBasti commented Feb 23, 2017 •

edited

tiran commented Feb 23, 2017

lslebodn commented Feb 23, 2017

tiran commented Feb 23, 2017

lslebodn commented Feb 23, 2017

pvoborni commented Feb 23, 2017

lslebodn commented Feb 23, 2017 •

edited

MartinBasti commented Feb 23, 2017

tiran commented Feb 23, 2017

MartinBasti commented Feb 23, 2017

lslebodn Feb 23, 2017

tiran Feb 23, 2017

lslebodn Feb 23, 2017

tiran Feb 23, 2017

MartinBasti Feb 23, 2017

lslebodn Feb 23, 2017 •

edited

tiran commented Feb 23, 2017

MartinBasti Feb 23, 2017

tiran Feb 23, 2017

MartinBasti commented Mar 2, 2017

Packaging: Add placeholder packages #472

Packaging: Add placeholder packages #472

Conversation

tiran commented Feb 16, 2017 • edited by MartinBasti

MartinBasti commented Feb 17, 2017

MartinBasti commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran commented Feb 17, 2017

HonzaCholasta commented Feb 20, 2017

MartinBasti commented Feb 20, 2017

tiran commented Feb 21, 2017

MartinBasti commented Feb 21, 2017

tiran commented Feb 21, 2017

tiran commented Feb 22, 2017

MartinBasti commented Feb 22, 2017

MartinBasti commented Feb 22, 2017

tiran commented Feb 22, 2017

MartinBasti commented Feb 23, 2017 • edited

tiran commented Feb 23, 2017

lslebodn commented Feb 23, 2017

tiran commented Feb 23, 2017

lslebodn commented Feb 23, 2017

pvoborni commented Feb 23, 2017

lslebodn commented Feb 23, 2017 • edited

MartinBasti commented Feb 23, 2017

tiran commented Feb 23, 2017

MartinBasti commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lslebodn Feb 23, 2017 • edited

Choose a reason for hiding this comment

tiran commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinBasti commented Mar 2, 2017

tiran commented Feb 16, 2017 •

edited by MartinBasti

MartinBasti commented Feb 23, 2017 •

edited

lslebodn commented Feb 23, 2017 •

edited

lslebodn Feb 23, 2017 •

edited