pypa / setuptools

Official project repository for the Setuptools build system
https://pypi.org/project/setuptools/
MIT License
2.52k stars 1.19k forks source link

Provide a better error when setup.py is missing #1710

Open cjerdonek opened 5 years ago

cjerdonek commented 5 years ago

In a directory with a valid pyproject.toml (that uses setuptools) but no setup.py, we still have this error. This however probably needs to be fixed in setuptools (since the error is now stemming from the setuptools PEP 517 backend).

@Casper-Oakley Would you be willing to look into making the corresponding fix over at setuptools for this? It'll need changes in setuptools/build_meta.py around line 82.

Originally posted by @pradyunsg in https://github.com/pypa/pip/pull/5926#issuecomment-441369683

pganssle commented 5 years ago

I think this is probably already fixed in master. PR #1675 has added support for setup.cfg-only projects, we just need to cut a new release.

cjerdonek commented 5 years ago

This issue is to provide a better error message in the case that the needed files are missing though (setup.py or I guess now setup.cfg). Is that also addressed? It didn’t look like the missing file(s) case was handled any differently in that PR, though I could be wrong.

pganssle commented 5 years ago

@cjerdonek I'm not sure you'll actually get an error if both setup.cfg and setup.py are missing, I think it will just successfully install the package with bad metadata:

Testing it out with the current setuptools version but a manually generated setup.py file equivalent to the auto-generated one that setuptools.build_meta creates, looks like it will successfully install:

$ pip wheel . -w dist --no-deps
Processing /tmp/setup_missing
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
    Preparing wheel metadata ... done
Building wheels for collected packages: UNKNOWN
  Building wheel for UNKNOWN (PEP 517) ... done
  Stored in directory: /tmp/setup_missing/dist
Successfully built UNKNOWN

It seems like that should probably throw an error because required metadata is missing, like the name of the package. Not sure if requiring a valid name would be a breaking change, though.

cjerdonek commented 5 years ago

Not sure if requiring a valid name would be a breaking change, though.

Since the new feature is only in master, wouldn't this already be breaking because of setup.py being missing?

cjerdonek commented 5 years ago

Or oh, are you just making a point as to where the error should / would need to happen in the case of this issue?

pganssle commented 5 years ago

Since the new feature is only in master, wouldn't this already be breaking because of setup.py being missing?

I just mean that neither setup.cfg nor setup.py currently requires you to pass it any information at all, so with the change on master, a project that consists of only a pyproject.toml file is equivalent to a pre-517 project with no setup.cfg and a setup.py that looks like this:

from setuptools import setup; setup()

Right now that's not an error, though I can't imagine anyone is doing it deliberately. Worst case scenario we could require that you either have a setup.cfg or setup.py, but I think I'd prefer a situation where we are throwing errors because the package it's generating has invalid metadata, since that covers both the case of "forgot to add setup.cfg" and the case of "forgot to add name to the metadata".

cjerdonek commented 5 years ago

Would this change need to happen before you make a new release, and is it a release blocker? (It seems like it would be better to make the change beforehand so that it won't become a breaking change post-release.)

I think I'd prefer a situation where we are throwing errors because the package it's generating has invalid metadata,

If the error handling is done this way, it would still be good to somehow include in the message that the file is missing (in the case they didn't provide a file), so that it's clearer to the user what the corrective action is. If the error just says something about "invalid metadata," it wouldn't necessarily be obvious if it's because they're missing a setup.py, etc. Ideally the error message would know whether a file was originally present (as opposed to an auto-generated one).

cjerdonek commented 5 years ago

One more comment: this is reminiscent of issue #1664 I submitted a PR for (https://github.com/pypa/setuptools/pull/1706) where currently setuptools does tell the user there is invalid metadata by saying there is a Missing 'Version:' header but without giving enough context like the path to the errant file. Even after that PR more could be done by distinguishing the case of the file being missing from the case of the file being present but with the header missing.

pganssle commented 5 years ago

Would this change need to happen before you make a new release, and is it a release blocker? (It seems like it would be better to make the change beforehand so that it won't become a breaking change post-release.)

It's hard for me to say. I am not sure it's particularly related to the "you may have a project without a setup.py" feature. I'm also not sure why people are trying to install things with no setup.py. The only reason I can imagine would be if setup.py were accidentally not included in the sdist, but that only happens with PEP 517 sdists in an earlier version of setuptools.

I think the metadata validation issue is probably separate from the concern about missing files, and the only related question is if we start raising errors due to missing metadata, should we try to detect that the reason the metadata is missing is that the file that you specify the metadata in is missing. I'm not really sure it will be a sufficiently common error that it's worth doing.

cjerdonek commented 5 years ago

I am not sure it's particularly related to the "you may have a project without a setup.py" feature.

Just to clarify, the potential breaking change I was referring to is throwing an error if both setup.cfg and setup.py are missing (whether that error be because of a missing file, or from missing metadata from an auto-generated file). If in the next release you start letting users have such projects, it would be a breaking change to start erroring out in that situation in a subsequent release.

if we start raising errors due to missing metadata, should we try to detect that the reason the metadata is missing is that the file that you specify the metadata in is missing.

I think this would lead to a better user experience, and it seems on the easy side to implement and test (e.g. easier than the PR I recently submitted), so it seems like it should be done IMO. It can even be the same exception class in both cases (e.g. MetadataError) instead of the current FileNotFoundError in one case as seen in https://github.com/pypa/pip/pull/5926 .

Worst case scenario we could require that you either have a setup.cfg or setup.py, but I think I'd prefer a situation where we are throwing errors because the package it's generating has invalid metadata,

I'd be happy to implement the former as a PR today, in which case the latter could be done as a subsequent PR. I'm not familiar enough with the code base to know how easy or hard the latter would be to do.