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

ez_setup.py accepts unused 'delay' parameters in several of its functions #173

Closed
ghost opened this issue Mar 23, 2014 · 12 comments
Closed
Labels
minor Needs Triage Issues that need to be evaluated for severity and status. proposal

Comments

@ghost
Copy link

ghost commented Mar 23, 2014

Originally reported by: jurko (Bitbucket: jurko, GitHub: jurko)


setuptools project's ez_script.py script contains several functions taking a delay or a similar delay-like parameter that effectively gets ignored in the end.

  • download_delay in use_setuptools() --> forwarded to _do_download()
  • download_delay in _do_download() --> forwarded to download_setuptools()
  • delay in download_setuptools() --> ignored

My suggestion would be to either remove those parameters or at least mark them as deprecated and scheduled for removal in some future setuptools version.

Best regards,
Jurko Gospodnetić


@ghost
Copy link
Author

ghost commented Mar 23, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


@jurko would you be willing to trace the code ancestry, maybe looking at setuptools 0.6 and distribute, to see if this parameter was ever used? If it was, it would be very useful to know when it was no longer honored, even if just for documentation purposes.

@ghost
Copy link
Author

ghost commented Mar 23, 2014

Original comment by jurko (Bitbucket: jurko, GitHub: jurko):


I don't think I've ever seen it honored. I just checked out the setuptools 1.0 release from https://pypi.python.org/pypi/setuptools/1.0 and that script has the same unused parameters. 😄

Hmmm, but 0.6c8 has something related to it:

#!python
            if delay:
                log.warn("""
---------------------------------------------------------------------------
This script requires setuptools version %s to run (even to display
help).  I will attempt to download it for you (from
%s), but
you may need to enable firewall access for this script first.
I will start the download in %d seconds.

(Note: if this machine does not have network access, please obtain the file

   %s

and place it in this directory before rerunning this script.)
---------------------------------------------------------------------------""",
                    version, download_base, delay, url
                ); from time import sleep; sleep(delay)

I'll try to track down exactly where it got changed...

@ghost
Copy link
Author

ghost commented Mar 23, 2014

Original comment by jurko (Bitbucket: jurko, GitHub: jurko):


Found it - removed in the distribute project.

  • commit: a09863d51b3040d8555bf665ff1c63ab601cc11f
  • date: 2009-09-08
  • description: simplifying the bootstrap process
  • author: tarek

Removed in the file distribute_setup.py.

Before the script really did sleep for a given number of seconds before initiating the setuptools egg download.

The default delay time was 15 seconds.

It could be specified externally (i.e. using a public interface) via use_setuptools() & download_setuptools().

And since I assume most installations wanted for their download to start as soon as possible, they would in general really specify that parameter and set it to 0 or None.

That means that some older package installations may still be doing that simply because 'that's the way it's always been done'.

If that is so, then simply removing this parameter would not be something you'd want to do without a depreciation period.

My suggestion, after seeing this, is that you output a very visible warning message about this parameter having no meaning, and how it should not be used as it has been marked as deprecated and will be removed in some future setuptools release.

And then schedule actually removing it at some later time, e.g. in a year or two. 😄

You can do the usage check safely by defining a local UnusedParam class, use that as the parameter's default value in use_setuptools() & download_setuptools() and check for that value & report a user warning in _do_download().

Hope this helps.

Best regards,
Jurko Gospodnetić

@ghost
Copy link
Author

ghost commented Mar 23, 2014

Original comment by jurko (Bitbucket: jurko, GitHub: jurko):


Sorry for much comment editing above - my initial response was sent to hastily, 😢 but their content should be correct now.

@ghost
Copy link
Author

ghost commented Mar 23, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Well done. Thanks for documenting this. I agree that adding a warning is the right thing to do at this stage. Feel free to send a pull request, or I'll get to it at some point.

@ghost
Copy link
Author

ghost commented Mar 23, 2014

Original comment by jurko (Bitbucket: jurko, GitHub: jurko):


This is a patch for ez_setup.py (based on the current BitBucket repository tip) that does the following:

  • Defines a new internal _DefaultDeprecatedParamValue class.
  • Defines a new internal _report_deprecated_parameter_usage() function displaying a very visible deprecated parameter usage warning to the user on sys.stderr.
  • Removes internal _do_download() function's unused download_delay argument.
  • Calls the new _report_deprecated_parameter_usage() function from use_setuptools()/download_setuptools() functions in case they get called with their download_delay/delay argument value specified.

I noticed that the internal _do_download() function is called from the project's tests/test_ez_setup.py script, so that script will need to be updated as well (seems simple enough - just remove one dummy value being passed to it in one of the tests). I tried updating it but am not quite sure how to know that I have not broken anything since calling that script directly has been reporting some failures even before my changes and it seems to produce a looot of noise when run.

Otherwise, my intention was to add some tests to it like the following (N.B. untested and requires further refactoring to avoid code duplication):

#!python
    def test_download_setuptools__delay_warning(self):
        tmpdir = tempfile.mkdtemp()
        saved_stderr = sys.stderr
        my_stderr = io.StringIO()
        sys.stderr = my_stderr
        try:
            download_setuptools(to_dir=tmpdir, delay=None)
            self.assertIn("WARNING  WARNING  WARNING  WARNING", my_stderr.getvalue(),
                "download_setuptools() delay argument usage warning not "
                "issued")
        finally:
            sys.stderr = saved_stderr

    def test_download_setuptools__no_delay_warning(self):
        tmpdir = tempfile.mkdtemp()
        saved_stderr = sys.stderr
        my_stderr = io.StringIO()
        sys.stderr = my_stderr
        try:
            download_setuptools(to_dir=tmpdir)
            self.assertNotIn("WARNING  WARNING  WARNING  WARNING", my_stderr.getvalue(),
                "download_setuptools() delay argument usage warning issued "
                "unexpectedly")
        finally:
            sys.stderr = saved_stderr

    def test_use_setuptools_delay_parameter__delay_warning(self):
        saved_stderr = sys.stderr
        my_stderr = io.StringIO()
        sys.stderr = my_stderr
        try:
            use_setuptools(download_delay=0)
            self.assertIn("WARNING  WARNING  WARNING  WARNING", my_stderr.getvalue(),
                "use_setuptools() download_delay argument usage warning not "
                "issued")
        finally:
            sys.stderr = saved_stderr

    def test_use_setuptools_delay_parameter__no_delay_warning(self):
        saved_stderr = sys.stderr
        my_stderr = io.StringIO()
        sys.stderr = my_stderr
        try:
            use_setuptools()
            self.assertNotIn("WARNING  WARNING  WARNING  WARNING", my_stderr.getvalue(),
                "use_setuptools() download_delay argument usage warning "
                "issued unexpectedly")
        finally:
            sys.stderr = saved_stderr

Without going much deeper into its implementation, it seems to me that this test script might have gone stale and has not been kept updated. Possibly because the regular setuptools test suite (run using setup.py test) does not include it. What hints in this direction is that its test_install() test seems to set some ez_setup.python_cmd attribute but there is no such attribute used anywhere else in the code-base. There is a ez_setup._python_cmd attribute, but not one named without a leading underscore.

Hope this helps.

Best regards,
Jurko Gospodnetić

@ghost
Copy link
Author

ghost commented Mar 23, 2014

Original comment by jaraco (Bitbucket: jaraco, GitHub: jaraco):


Thanks for this. Yes, I believe there is a ticket open for the tests not run. I appreciate you taking a stab at adding tests for the issue. Much appreciated.

@ghost
Copy link
Author

ghost commented Mar 24, 2014

Original comment by pje (Bitbucket: pje, GitHub: pje):


FYI, the original reason for having a delay was to allow the user to prevent ez_setup.py from even starting to download code over the internet, as a side effect of manually installing a package from source. At the time the code was written, some people would really freak out about this sequence of events:

  1. I download package X, untar it, and run setup.py install
  2. It suddenly starts accessing the internet and downloading code -- the horror!

The banner was placed there in order to give people a warning and explanation of what was happening, while also giving them a chance to abort the process and do it manually, instead of either freaking them out or else hanging on an absent network connection. Both scenarios (i.e "WTF is this" and "why doesn't your crappy software handle disconnected installs") were complained about bitterly when ez_setup first landed -- so I added the warning and delay, and the complaints got less frequent and considerably less bitter.

I'm not sure why distribute removed this; maybe they thought that if you were running their ez_setup then you knew all about this already, and just didn't think about end users in the third-party install scenario, where they are running some random setup.py and not expecting downloading to take place.

@ghost
Copy link
Author

ghost commented Mar 24, 2014

Original comment by jurko (Bitbucket: jurko, GitHub: jurko):


Ok, I'm a bit confused here. Was that not something that package X was supposed to take care of?

I mean - it decided to use ez_setup.py and enable automated download. If a user finds this wrong - why should he be 'bitter' towards setuptools? Should he not take up the issue with whoever is maintaining package X?

I don't really see someone would find this to be the setuptools' fault. It clearly advertises what ez_setup.py is for and what you get when you use it.

Package X can always choose to not use ez_setup.py or to launch its install functionality with a preloaded local installation so the whole download issue is avoided.

Or is there something I'm missing here?

Best regards,
Jurko Gospodnetić

@ghost
Copy link
Author

ghost commented Mar 24, 2014

Original comment by pje (Bitbucket: pje, GitHub: pje):


Yes, people took up the issue with package X, by telling them not to use setuptools. ;-)

As for package X bundling setuptools, the whole idea behind ez_setup.py was to make it possible for package X not to deal with bundling anything besides ez_setup.py, in order to give the user a one-step (or nearly so) installation process.

So, if package X's maintainer has to jump through these hoops, using ez_setup became more of a pain, and the whole idea was for it to reduce the pain for both the developer and the end user.

It's possible that at this point, almost a decade later, that "everybody knows" running "setup.py install" might cause code to be downloaded from the internet. What's more, it's much more common for people to already have pip or easy_install already on their system in the first place, so the issue never comes up.

I'm not saying the message or the delay should be kept, per se; just explaining why it was there in the first place, to make sure the use case was known and understood, so that an informed decision can be made.

@ghost
Copy link
Author

ghost commented Mar 25, 2014

Original comment by jurko (Bitbucket: jurko, GitHub: jurko):


Here are some more random ramblings on the subject - hopefully they coalesce into something concrete by the time I'm done writing them. 😄

First of, if a package X can be written without its setup and/or runtime requiring setuptools, and can do so easily - it really should. Nuff' said... 😄

However, setuptools addresses some real problems encountered when preparing complex project setup & configuration procedures, e.g. automatically running py2to3 source code conversions, collecting a list of modules to be included in a distribution, managing dependencies, organizing a clean externally managed entry point repository accessible via a clean standardized API, etc.

If a project encounters such problems, it can resolve them itself and pay the price of developing & maintaining that code base, or it can opt to use setuptools. The tradeoff here is for package X maintainers to make. Personally, now that setuptools development is active again, I generally opt to use setuptools freely. 😄

If a project opts to use setuptools as its dependency - that dependency needs to be installed somehow, same as with any other dependency - with the cost paid by project X and/or the user. Available options are discussed below.

#Project X perspective#

##1. project X not having dependencies##

Difficult for non-minimalistic projects, as it can require lots of custom coding & maintenance.

  • project X pays a potentially hefty price
  • project X has a larger distribution then necessary
  • projects X & Y have duplicate functionality which all needs to be installed in the user's environment (more space used, more variety, less centralized issue resolution, ...)

##2. user downloading/installing project X and its dependencies##

  • project X needs to document this (untested documentation maintenance issues)
  • user pays an extra price during installation - multiple downloads, different installation procedures to follow, more manual labor
  • project X can assist with this by preparing a script to download the needed dependencies (work duplicated in project Y)

##3. project X distribution including its dependencies##

  • project X distributions become larger
  • project X needs to do extra work in its distribution packaging procedure
  • in case of setuptools - it can help project X by implementing a utility script that would make this easier for project X, similar to how ez_setup.py makes it easier to implement downloading on-demand during installation
  • if no package management dependency is included - project X needs to pay a hefty price of detecting existing installations and deciding whether or not to install a particular bundled dependency (eeek...)

##4. project X installation downloads and installs the dependencies##

This is the scenario that setuptools project's ez_setup.py utility script helps with.

  • this does require additional downloads during a setup, though any other dependencies require additional downloads as well so setuptools is nothing special here
  • user can avoid the additional download by downloading things manually first

All in all - someone, somewhere needs to pay the piper. 😄

#User perspective#

Looking at this from the user's perspective - a user can want different things from package X, and as a user - I want it all! 😄

  • I want to be able to run a simple one-shot installation that downloads whatever it needs from the net.
  • I want to be able to run an installation with any downloaded resources removed after the installation.
  • I want to be able to run an installation with any downloaded resources kept locally so another installation can easily reuse them and avoid having to download them again.
  • I want to be able to run a simple one-shot installation that downloads whatever it needs but only from specific repositories, probably ones I control.
  • I want to be able to easily determine all the dependencies for a project without installing it, so I can download them manually.
  • I want to be able to download all the dependencies for a project (from the net or from specific repositories) without installing the project or its dependencies.
  • I want to be able to package all the required dependency installations together with the project installation so I can transport & install them all off-line.
  • I want to be able to run an installation in such a way that I am sure it will not attempt to contact the net or any external sources, and have it report back what, if anything, is missing.
  • I want to be able to run an installation and be sure some dependency installation sources are used before others.
  • I want to be able to run an installation and limit sources & source usage rules for specific dependencies only.
  • I want to be able to detect whether all the required dependencies have already been installed on the system without installing anything or any additional downloads, and with a warning if additional downloads would be required to fully answer my query.
  • I want to be able to install a package without installing any of its run-time dependencies.
  • I want to be able to install a package without installing its run-time dependencies whose installations would require additional unsanctioned downloads, but with a precise listing of all the missing dependencies.
  • I want to be able to install a package and explicitly change its dependencies (since the developer obviously did not know what he was doing when he specified that nonsense... 😄)

and all such use cases should be allowed. 😄

Project X should be able to decide the 'default behaviour' most suitable for its users, but users should be able to specify their desired behaviour explicitly.

The horror @pje spoke of would relate to use cases where a user did not want an extra download. My first instinct tells me to attack that problem by instructing/helping project X provide command-line options explicitly choosing its behaviour, and selecting the 'default' it finds suitable for its users. I guess the 'show message & delay' technique can be supported as well, but I would definitely not make it the default behaviour 'when project X chooses nothing else'.

Since setuptools is a package management solution, I guess project X could make a 'simple call' to one of its utility scripts like ez_setup.py, passing the desired default behaviour. Project X could do its own command-line argument parsing and pass its results in or it could let this script do that work as well. It would then make sure it uses compatible behaviour for managing itself as a dependency and any other dependencies. Then a 'simple' but still 'batteries included' solution could be had for a typical project X, but I am not really sure how complicated that would get on the setuptools side or how all this and the different command-line arguments introduced would play along with other package managers out there.

I guess one thing that does make the ez_setup.py script's case simpler than the general dependency management scenario is that when that script is used, setuptools is definitely an install-time dependency so such a utility script does not need to handle all the use cases above. If setuptools can not be installed - presto - installation exits and there is no need to consider cases where project X should be installed without setuptools.

Hope this helps.

Best regards,
Jurko Gospodnetić

@pganssle pganssle added the Needs Triage Issues that need to be evaluated for severity and status. label Oct 19, 2018
@abravalheri
Copy link
Contributor

Hello, I think we should be able to close this considering the fact that ez_setup has been deprecated and superseded by pip (or other mechanisms)...

If anyone would like to reopen this issue, please feel free to comment bellow with more information or other use cases that we might be missing 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Needs Triage Issues that need to be evaluated for severity and status. proposal
Projects
None yet
Development

No branches or pull requests

2 participants