search

pypa / flit

Simplified packaging of Python modules
https://flit.pypa.io/
BSD 3-Clause "New" or "Revised" License
2.15k stars 131 forks source link

Traceback exposed if upload fails #558

Open palao opened 2 years ago

palao commented 2 years ago

Hello, I got a traceback in flit while trying to publish a package:

$ flit publish
Found 360 files tracked in git                                                                                                                                                              I-flit.sdist
Built sdist: dist/MyPackage-0.5.0.tar.gz                                                                                                                                   I-flit_core.sdist
Copying package file(s) from /tmp/tmpdw358fz9/MyPackage-0.5.0/mypackage                                                                                       I-flit_core.wheel
Writing metadata files                                                                                                                                                                 I-flit_core.wheel
Writing the record of files                                                                                                                                                            I-flit_core.wheel
Built wheel: dist/mypackage-0.5.0-py3-none-any.whl                                                                                                                         I-flit_core.wheel
Using repository at https://upload.pypi.org/legacy/                                                                                                                                        I-flit.upload
Uploading dist/mypackage-0.5.0-py3-none-any.whl...                                                                                                                             I-flit.upload
Traceback (most recent call last):
  File "/home/palao/.virtualenvs/flit-py39/bin/flit", line 8, in <module>
    sys.exit(main())
  File "/home/palao/.virtualenvs/flit-py39/lib/python3.9/site-packages/flit/__init__.py", line 185, in main
    main(args.ini_file, repository, args.pypirc, formats=set(args.format or []),
  File "/home/palao/.virtualenvs/flit-py39/lib/python3.9/site-packages/flit/upload.py", line 268, in main
    do_upload(built.wheel.file, built.wheel.builder.metadata, pypirc_path, repo_name)
  File "/home/palao/.virtualenvs/flit-py39/lib/python3.9/site-packages/flit/upload.py", line 246, in do_upload
    upload_file(file, metadata, repo)
  File "/home/palao/.virtualenvs/flit-py39/lib/python3.9/site-packages/flit/upload.py", line 239, in upload_file
    resp.raise_for_status()
  File "/home/palao/.virtualenvs/flit-py39/lib/python3.9/site-packages/requests/models.py", line 960, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 400 Client Error: Invalid value for requires_dist. Error: Can't have direct dependency: 'somedep @ git+https://github.com/whoever/somedep.git' for url: https://upload.pypi.org/legacy/

the problem is that PyPI rejects it (seems to be related to their policy, see #555). However my question is: is the display of the traceback a feature, or a bug? Should I make a PR?

takluyver commented 1 year ago

In that case, the traceback clearly isn't helpful - there's a problem with the package we're uploading, not the uploader code. I'm wary of hiding tracebacks in general, though, because they can be useful to understand what's going on.

Some possible ways to address that:

  1. Extend the checks we do when building to cover such cases - the idea is that we catch things like this before you try to upload to PyPI. Checking requirements is done here. This one is tricky, however, because a requirement specified by direct URL may be OK for a package you're using privately, it just can't go on PyPI.
  2. Hide the traceback specifically when we get status code 400, which hopefully means PyPI is telling us about something wrong with the package. :crossed_fingers:
  3. Hide the traceback normally but have some way to turn it on (this is inspired by Rust). The downside is that it's a pain for rare errors where it might be fine when you run it again.
takluyver commented 1 year ago

P.S. Sorry this one went unanswered for so long

palao commented 1 year ago

Thank you, @takluyver for answering and for sharing this clear analysis. I really appreciate your comments: they are very insightful. No worries: it is not a critical thing... still I think it should be fixed, and I agree, the big problem I see here is that the traceback can be confusing. The error is correctly describing what happens, but I needed a while to understand that flit was doing the right thing and that it was my ignorance about the PyPI policy what caused the problem.

I would be in favour of not doing a very clever checking. I don't see a practical way to address your point 1. Your reasoning about hiding tracebacks is convincing (your point 3): it can be a painful default. Still it is more appealing than point 1. I use this strategy sometimes.

My winner is point 2: if flit is doing things right and the hosting is legitimately answering "No, you can't do that", I think this is a case that should be properly addressed by flit. Simply preventing the exception to propagate and printing an error similar to what's displayed in the last line should be enough (maybe adding some more generic explanation).

What do you think? Should I do a PR?

takluyver commented 1 year ago

I think that's reasonable, yeah.

What we do for other scenarios like this is that the main CLI code catches a group of errors where we just want to display the message, not the traceback - e.g. here it is for flit build:

https://github.com/pypa/flit/blob/f5704ea31f0fcc579b8518ea85d641651cba4f71/flit/__init__.py#L178-L182

So I would define a new exception like UploadRejectedError, transform HTTP 400 errors into this, and then apply a similar mechanism to flit publish.

palao commented 1 year ago

I like it. It makes sense.