Other project zest.releaser hooks pollute my own release workflow

[hgrecco]

Something very weird happened when I was releasing my own project (not related with astropy):

  File "/Users/username/anaconda/bin/fullrelease", line 11, in <module>
    sys.exit(main())
  File "/Users/username/anaconda/lib/python3.5/site-packages/zest/releaser/fullrelease.py", line 32, in main
    postreleaser.run()
  File "/Users/username/anaconda/lib/python3.5/site-packages/zest/releaser/baserelease.py", line 307, in run
    self._run_hooks('middle')
  File "/Users/username/anaconda/lib/python3.5/site-packages/zest/releaser/baserelease.py", line 302, in _run_hooks
    utils.run_hooks(self.setup_cfg, which_releaser, when, self.data)
  File "/Users/username/anaconda/lib/python3.5/site-packages/zest/releaser/utils.py", line 579, in run_hooks
    run_entry_points(which_releaser, when, data)
  File "/Users/username/anaconda/lib/python3.5/site-packages/zest/releaser/utils.py", line 595, in run_entry_points
    plugin(data)
  File "/Users/username/anaconda/lib/python3.5/site-packages/astropy/utils/release.py", line 260, in postreleaser_middle
    _update_setup_py_version(data['dev_version'])
  File "/Users/username/anaconda/lib/python3.5/site-packages/astropy/utils/release.py", line 269, in _update_setup_py_version
    output.write(line.decode('utf-8'))
AttributeError: 'str' object has no attribute 'decode'

It seems that astropy's zest releaser hook is polluting my own release workflow. There is something very wrong about this

[hgrecco]

One more thing: I found out because astropy hook failed. This might be happening for other people as well and something they are not aware might be running while they release.

[mauritsvanrees]

Well, hooks can be global or local. Both can be valid use cases and are supported.

zest.pocompile has a global hook that is active when releasing any package: it compiles .po files to .mo files. Some other packages that use a hook for global use, are pyroma and check-manifest.

zest.releaser itself has a local hook to prepare some documentation only when zest.releaser itself is being released. What makes it local, is the data['name'] check in this line of the hook:
https://github.com/zestsoftware/zest.releaser/blob/6.6.2/zest/releaser/preparedocs.py#L16

The only thing that makes a hook local, is such a check on data['name']. Hooks are called when the package with the hook is installed in the same python/virtualenv as zest.releaser.

I don't know astropy so I cannot say if its hook is supposed to be global or local.
As for the specific error, this package might need to check for six.text_type if it wants to support both Python 2 and 3, something like this:

zest.releaser/zest/releaser/utils.py

Line 645 in 8c7489a

if not isinstance(stdout_output, six.text_type):

[hgrecco]

Thanks for your response. But is kind of annoying that a package can install a global hook without my consent. If there is no way to limit this, I would like to be able opt-in in my package to use a global hook.

[hgrecco]

By the way, astropy comes with anaconda. So a lot of Python users might be having this problem.

[reinout]

I've opened a ticket for astropy itself with @mauritsvanrees' tip included: astropy/astropy#4633

It is my guess, too, that the problem should be fixed there.

[hgrecco]

Thanks for looking into this but please consider rethinking the strategy in zest releaser. I think is utterly broken that a project with a bad hook can make my zest installation unusable. I think it would be better if projects can install and a advertise global hooks, but you need to opt-in for them.

[reinout]

I don't think I want to switch to an opt-in mechanism. Most of the hooks I've seen in use are intended to be used for all projects. If the user installs them, the user intends them to be used.

Otherwise you have to add code/configuration to every single project.

But you are probably right that it is broken if a bad hook can make everything unusable. @mauritsvanrees, what about a bare try/except around hooks that catch everything? It won't help against already-mis-modified data, but at least it'll help when they error on something.

Would that be safe to do? Or are there other consequences? Are there hooks that depend on exceptions bubbling up?

[hgrecco]

That is exactly the point; a user install a package to use the package itself, not necessarily package zest releaser hooks. The hooks are not advertised nor the user is able to pip install the package without them.

I like the opt-in idea. I think that it could be done easily in the setup.cfg or at least asking during the release process as it is done for pyroma.

(It is different with things like pyroma since that package purpose is to help you with the release process)

[reinout]

And my point is two-fold:

• If a package has a hook just for that package, it should do the data['name'] = myself check. If not, that is a bug. We could catch that partially by a try/except around the whole hook.
• There are several packages that only install a hook. Those are are often intended to be installed globally, for instance to adhere to company-wide naming standards or company-wide checks. This is a valid usecase, so I want to keep this as a possibility.

Removing global hook support because there's a bug in one package: I think there'll be a lot of outcry if we were to do that :-)

[hgrecco]

If you do not agree to opt-in for global hook, how about this:

1. Provide a way to inspect hooks (fullrelase --list-hooks). I would like to see what is running during my release process. (Maybe this exists but I was not able to find it in the docs)
2. Make the mechanism for local hooks very distinct from global hooks. For example, ask the user to put all local hooks in a zest_releaser.py file next to setup.py

In that file, functions should be named according to the hook you want to call. For example:

def prereleaser_middle(data):
    # do something here

Another reason for this is that the release related code does not need to be inside your package.

What sounds wrong to me is that a project need to add this to make it local.

    if data['name'] != 'my.package':
        return

Local should be the default for a hook, global should require more code.

[reinout]

Local file instead of the existing hooks: no. Way too much work and I don't want to turn that into a completely different mechanism.

The --list-hooks is a nice one!

The "if data" trick is a very small thing you have to add. Now that the issue cropped up, in hindsight, a local=True keyword argument would have been a good idea. But for backwards compatibility it is irritating to add it now. So the "if data" trick is there to stay (unless @mauritsvanrees has a bright idea).

You seem to be intent on getting rid of global hooks and you think they're bad and evil. To put your problem into perspective:

• Hooks exist for 4 years or so. This is the first time this issue cropped up. The current hook mechanism cannot be completely bad that way, right?
• It is a bug in astropy. They're missing two lines.
• The worst effects of the bug can be prevented in zest.releaser with a simple try/except.
• The issue only crops up if the astropy package is installed globally (or in the virtualenv/anaconda/whatever where you're also releasing your code).

So, to me it looks like a relatively isolated case. It isn't worth breaking many existing packages by making backwards-incompatible changes. (It is worth making some fixes in zest.releaser, of course :-) )

[hgrecco]

It is not that the data trick is big or small: is just that I think that the case with more consequences should require more code. I think that the worst effect is actually a package tampering with my release process.

But in any case, thanks for taking the time for reading and answering. And most importantly for your time in this very useful package.

[mauritsvanrees]

I still think the best is for you to create a virtualenv for zest.releaser with only the extra packages that you want. All should work fine then.

I agree with Reinout. At least ninety percent of the hooks I use are global hooks, meant for every package. I happily use them. It is seldom that I see a local one.

We are going to keep that. Whatever we do, hooks that are currently operating globally, will not be switched off without any action by the user. But we can think of adding command line or config options.

Something like --list-hooks: sounds good. Or maybe that should be a command release-show-hooks. But with an option we can let it show just the relevant hooks, so do not list the postrelease hooks when doing prerelease --list-hooks.

Doing a try/except seems fine too. We are currently not checking a return value or catching any exceptions. When a hook currently raises an exception, it bubbles up and the script quits. When we start catching this, that is a relatively big change, but seems safe and good to me. When we catch an error, we should print the traceback, so the add-on author has a chance to see what goes wrong. And we should then ask the user if she wants to continue or quit. (Or possibly retry the hook, we have code for that.)

Maybe an option skip-all-hooks, or a list skip-hooks which you add in setup.cfg or ~/.pypirc. That gives an explicit opt-out. To skip a hook from astropy you would need to provide its full name, which from the looks of it would be something like this: zest.releaser.prerelease.middle:astropy.release.prereleaser.middle
I am no fan of this, but it might be an option to consider.

Any chance that you could work on a pull request for the --list-hooks option and the try/except? :-)

[hgrecco]

I will look into it and let you know

[reinout]

They fixed it in astropy: astropy/astropy#4650 It should end up in future releases.

Apparently there are several entry points and all the other ones already had the if data['name'] !=astropy trick. So I'm quite convinced that this trick + documentation is already enough.

The --list-hooks of course is a feature that seems very useful, though.

[gforcada]

Not sure if I should open a new report... here goes my problem:

I made freitag.releaser, a wrapper around plone.releaser (which in turn is a wrapper around zest.releaser, yay!)

So plone.releaser installs quite a lot of entry points, which are fine and must be global for plone's purpose but that are getting on the way of my workflow with freitag.releaser.

So could there be a way to disable and/or override them? Right now I just forked plone.releaser and removed the entry points.

[mauritsvanrees]

Yeah, this has come up a few times, see also #173. That one is more focused, so I will add an idea there.