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

cabal upload: support package candidates #3419

Merged
merged 5 commits into from May 11, 2016

Conversation

Projects
None yet
7 participants
@bennofs
Copy link
Collaborator

bennofs commented May 10, 2016

This adds support for uploading package candidates by introducing a new
flag to cabal upload, --candidate. With this flag, cabal upload will
upload all supplied tarballs as package candidates. If combined with --documentation, the
flag can also be used to upload documentation for a package candidate to
hackage.

The PR also changes cabal upload to display warnings that the server generates.
This is in particular useful with candidates. I'm not sure whether we should leave that behaviour
by default or only enable it if uploading candidates, because some of the warnings are not so useful,
like

Exposed modules use unallocated top-level names: Generics GHC

Currently, I enabled it for all package uploads, regardless of whether they are candidates or not.

@bennofs bennofs force-pushed the bennofs:upload-candidate branch from 9f6c971 to a272b63 May 10, 2016

@bennofs

This comment has been minimized.

Copy link
Collaborator

bennofs commented May 10, 2016

I tested this PR with a local hackage instance from haskell/hackage master.

notice verbosity "Ok"
(code,err) -> do
notice verbosity $ "Error uploading documentation "
++ path ++ ": "
++ path ++ ": "

This comment has been minimized.

@23Skidoo

23Skidoo May 10, 2016

Member

Unintentional?

@@ -159,10 +160,19 @@ handlePackage :: HttpTransport -> Verbosity -> URI -> Auth
handlePackage transport verbosity uri auth path =
do resp <- postHttpFile transport verbosity uri path auth
case resp of
(200,_) ->
notice verbosity "Ok"
(code,warnings) | code `elem` [200, 204] -> do

This comment has been minimized.

@23Skidoo

23Skidoo May 10, 2016

Member

Needs a comment explaining when we can expect response code 204.

This comment has been minimized.

@bennofs

bennofs May 11, 2016

Collaborator

@23Skidoo Hmm, it means 204 NoContent and seems like it is only sent when uploading documentation for a package candidate, so it is not necessary in this particular case since the function is only used for packages. But since there is similar code in the function to upload docs, i thought thtat the response code makes sense here as well. Perhaps we should just accept any 20x code, since they all designate a successful operation?

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented May 10, 2016

Looks good, thanks for working on this.

@23Skidoo 23Skidoo added this to the cabal-install 1.26 milestone May 10, 2016

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented May 10, 2016

Fails on Travis due to -Werror. Looks like you forgot to update D.C.Config.

@hvr

This comment has been minimized.

Copy link
Member

hvr commented May 10, 2016

As mentioned already in #3418, I strongly suggest to have cabal upload upload candidates by default.
In fact, I never use cabal upload anymore, because it's too easy to mess-up and have to upload a fixup release (or perform a .cabal-edit) rightaway. Instead I use hackage-cli push-candidate.

I also regularly notice new users spam Hackage with superfluous previewing/draft uploads only because they weren't aware of Hackage's candidate-uploads feature (also, uploading broken releases can result in cleanup-debt that's not always addressed). Moreover, we had already a few cases of users uploading code that wasn't supposed to be public to Hackage and who were then surprised that they couldn't delete it again easily. I'd argue that it's far too easy to publish something to Hackage by accident. Once a package is published to the Hackage index it gets mirrored and persisted forever. So IMO the act of uploading a package into the public index ought to require an explicit --publish flag to make this a more conscious act and remind the user he's going to publish something globally/publicly.

Moreover, cabal upload should emit a visible information that something was uploaded as candidate, and give some guidance how to go from there (e.g. provide an URL to preview the result).

PS: See also haskell/hackage-server#481 & haskell/hackage-server#461

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented May 10, 2016

@bennofs If it's not too much too trouble, it'd be nice if @hvr's suggestions were implemented. Or if not, it can be of course done later.

@bennofs bennofs force-pushed the bennofs:upload-candidate branch from a272b63 to b6d9b23 May 11, 2016

bennofs added some commits May 10, 2016

cabal upload: support package candidates
This adds support for uploading package candidates by introducing a new flag to `cabal upload`,
--candidate. With this flag, `cabal upload` will upload all supplied tarballs as package candidates.
If combined with `--documentation`, the flag can also be used to upload documentation for a package
candidate to hackage.

Fixes #3418
upload: handle 204 response and display warnings
When uploading package candidates, the hackage server will return 204 responses in some cases.
The upload was still successfull in that case though.

We now also display warnings that the hackage server gives when a package is uploaded.
upload: more detailed message on successful upload
The message now includes a link to the package page and says
whether the package was uploaded as a candidate or not.

@bennofs bennofs force-pushed the bennofs:upload-candidate branch from b6d9b23 to f5d9221 May 11, 2016

@bennofs

This comment has been minimized.

Copy link
Collaborator

bennofs commented May 11, 2016

Ok, I implemented the suggest changes. The only question remaining is about the 204 error code that hackage-server seems to return for doc upload. Should we treat all 20x error codes as OK?

@dcoutts

This comment has been minimized.

Copy link
Member

dcoutts commented May 11, 2016

Oh yes please! Thanks @bennofs!

(@23Skidoo I've not reviewed the code)

As for result code 204, that is a common result for a POST where there is no particular content to return (e.g. no html page to display no errors etc). So I'm not sure about accepting all 2xx codes but 204 is perfectly fine here.

https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#2xx_Success

@23Skidoo 23Skidoo merged commit 1c9582e into haskell:master May 11, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented May 11, 2016

Merged, thanks!

@hvr

This comment has been minimized.

Copy link
Member

hvr commented May 12, 2016

yay... now I can start using cabal upload again... =)

@bennofs bennofs deleted the bennofs:upload-candidate branch May 12, 2016

@hvr

This comment has been minimized.

Copy link
Member

hvr commented May 12, 2016

@bennofs @dcoutts we may want to accept 303-responses as well; uploading a candidate returns a 303 pointing to the URL where the candidate can be previewed with a webbrowser...

@bennofs

This comment has been minimized.

Copy link
Collaborator

bennofs commented May 13, 2016

@hvr oh does it? I wonder why that never happened in my tests with a local hackage instance...

@hvr

This comment has been minimized.

Copy link
Member

hvr commented May 13, 2016

@bennofs here's what happens with the --http-transport=plain-http:

POST /packages/candidates HTTP/1.1
Host: hackage.haskell.org
User-Agent: cabal-install/1.25.0.0 (linux; x86_64)
Authorization: Digest XXXXXXXXXXXX
Content-Type: multipart/form-data; boundary=18186f18eaa610
Content-Length: 9484
Accept: text/plain

Recovering connection to hackage.haskell.org
Received:
HTTP/1.1 303 See Other
Server: nginx/1.8.1
Date: Fri, 13 May 2016 10:09:41 GMT
Content-Type: text/plain
Transfer-Encoding: chunked
Connection: keep-alive
Location: /package/cryptohash-md5-0.11.7.2/candidate
Content-Length: 0

303 - redirect
Redirecting to
http://hackage.haskell.org/package/cryptohash-md5-0.11.7.2/candidate ...
Sending:
GET /package/cryptohash-md5-0.11.7.2/candidate HTTP/1.1
Host: hackage.haskell.org
User-Agent: cabal-install/1.25.0.0 (linux; x86_64)
Content-Length: 0
Authorization: Digest   XXXXXXXXX
Content-Type: multipart/form-data; boundary=18186f18eaa610
Accept: text/plain

Recovering connection to hackage.haskell.org
Received:
HTTP/1.1 200 OK
Server: nginx/1.8.1
Date: Fri, 13 May 2016 10:09:41 GMT
Content-Type: text/html; charset=utf-8
Transfer-Encoding: chunked
Connection: keep-alive
Content-Length: 4984

Package successfully uploaded as candidate. You can now preview the result at
'http://hackage.haskell.org/package/cryptohash-md5-0.11.7.2/candidate'. To
publish the candidate, use 'cabal upload --publish'.
Warnings:
- OK

with --http-transport=curl however:

$ cabal --http-transport=curl upload dist/cryptohash-md5-0.11.7.2.tar.gz -v3
Searching for curl in path.
Found curl at /usr/bin/curl
Hackage password: 
Uploading dist/cryptohash-md5-0.11.7.2.tar.gz...
("/usr/bin/curl",["--config","-","https://hackage.haskell.org/packages/candidates","--form","package=@dist/cryptohash-md5-0.11.7.2.tar.gz","--write-out","\n%{http_code}","--user-agent","cabal-install/1.25.0.0 (linux; x86_64)","--silent","--show-error","--header","Accept: text/plain"])
Error uploading dist/cryptohash-md5-0.11.7.2.tar.gz: http code 303
@bennofs

This comment has been minimized.

Copy link
Collaborator

bennofs commented May 13, 2016

@hvr ok, this is weird:

$ cabal-install/dist/build/cabal/cabal --http-transport=curl --config-file=/data/code/acme-realworld/cabalconfig  upload /data/code/acme-realworld/dist/acme-realworld-0.1.1.tar.gz 
Uploading /data/code/acme-realworld/dist/acme-realworld-0.1.1.tar.gz...
Package successfully uploaded as candidate. You can now preview the result at
'http://localhost:8080/package/acme-realworld-0.1.1/candidate'. To publish the
candidate, use 'cabal upload --publish'.
Warnings:
- Exposed modules use unallocated top-level names: Acme
@hvr

This comment has been minimized.

Copy link
Member

hvr commented May 13, 2016

@bennofs does it really use curl? I see you pass it a --config-file... does -v3 tell you which transport it's really using?

@bennofs

This comment has been minimized.

Copy link
Collaborator

bennofs commented May 13, 2016

@hvr Yeah it definitely does use curl. Looks like hackage.haskell.org is running code different from hackage-server master, which is what I'm using for local testing.

@hvr

This comment has been minimized.

Copy link
Member

hvr commented May 13, 2016

@bennofs or maybe your curl behaves differently than mine? :-)

@bennofs

This comment has been minimized.

Copy link
Collaborator

bennofs commented May 13, 2016

I now tried with hackage.haskell.org as well, and it gives a 303 error like you've experienced. So it indeed is a difference between local-hackage and hackage.haskell.org.

@dcoutts

This comment has been minimized.

Copy link
Member

dcoutts commented May 13, 2016

See haskell/hackage-server@0939b25 The behaviour is new. That change will be deployed RSN.

@ezyang ezyang modified the milestones: cabal-install 2.0, 2.0 (planned for next feature release) Sep 6, 2016

@sol

This comment has been minimized.

Copy link
Member

sol commented Feb 15, 2018

no comment

@sol

This comment has been minimized.

Copy link
Member

sol commented Feb 15, 2018

but for reference, this breaks existing clients https://github.com/travis-ci/dpl/blob/master/lib/dpl/provider/hackage.rb

@gbaz

This comment has been minimized.

Copy link
Collaborator

gbaz commented Feb 15, 2018

what do you mean? do you mean that existing clients that used to upload directly will now upload as candidates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment