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

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

Open
cjerdonek opened this issue Feb 27, 2019 · 10 comments
Open

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

cjerdonek opened this issue Feb 27, 2019 · 10 comments
Labels
enhancement Needs Triage Issues that need to be evaluated for severity and status.

Comments

@cjerdonek
Copy link
Member

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 pypa/pip#5926 (comment)

@pganssle
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

pganssle commented Feb 27, 2019

@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.

@pganssle pganssle added enhancement Needs Triage Issues that need to be evaluated for severity and status. labels Feb 27, 2019
@cjerdonek
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

pganssle commented Feb 27, 2019

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
Copy link
Member Author

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
Copy link
Member Author

cjerdonek commented Feb 27, 2019

One more comment: this is reminiscent of issue #1664 I submitted a PR for (#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
Copy link
Member

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
Copy link
Member Author

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 pypa/pip#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.

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

No branches or pull requests

2 participants