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

Implement `cabal upload --doc` #2890

Merged
merged 11 commits into from Nov 5, 2015

Conversation

Projects
None yet
2 participants
@bennofs
Copy link
Collaborator

bennofs commented Oct 26, 2015

This is the second part that was still remaining for the implementation of #2080. I still need to test this on powershell though, I'll report back when I've done that.

bennofs added some commits Oct 16, 2015

HttpUtils: add support for PUT file uploads
This patch was tested with curl, wget and plain-http, but it's still untested on powershell.
The wget implementation will only work if the available wget supports the `--method` switch.
Implement `cabal upload --doc`
This will build (if not given as argument) a doc tarball and upload it to
hackage.

Closes #2080
@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Oct 27, 2015

Thanks for working on this! Can you look into what's causing the Travis failure?

@bennofs

This comment has been minimized.

Copy link
Collaborator

bennofs commented Oct 27, 2015

Ok, the upload works on windows, but cabal still reports an error (this is related to the powershell code, which doesn't deal with non-empty response bodies well). There is also some misbehaviour (reporting failure while the upload actually worked) when using curl. I'll look into those two issues, after that the PR should be ready to merge.

About the travis build, it seems that there is no longer any failure? Was it restarted?

bennofs and others added some commits Oct 27, 2015

Benno Fünfstück
HttpUtils: fixes for powershell transport
This commit refactors the powershell transport to avoid duplication and
also fixes a few problems with it:

* We now run powershell with the ExecutionPolicy bypass, is required when
  the default security policy disallows executing unsigned scripts
  (like our script)
* The powershell script itself has been refactored to behave more like
  curl. In particular, it now prints HTTP status code errors in a nicer
  way instead of failing with a PowerShell Exception + backtrack. We also
  now only print the exception message when we get any other exception,
  since the user is likely not familar with the powershell script at all
  and the other information is thus only confusing and not helpful at all.
* We now handle the case where the server returns some message after a
  POST or a PUT correctly. Previously, that resulted in the raw bytes
  being written to stdout before the HTTP status code, which confused the
  simplistic parser. We now always write the HTTP status code first and
  decode the bytes as UTF8 before sending them to stdout as well.
HTTPUtils: curl: correct handling of text response
This fixes an issue in the curl transport when a text response is returned
that doesn't end with a newline from a POST or PUT request. The code expects
the HTTP status code to be on a new line, but that is not the case if the response
doesn't have a trailing new line. We fix this by always printing a new line before
printing the status code. To avoid duplicate new lines, we now strip trailing empty
lines at the end of the response.
@bennofs

This comment has been minimized.

Copy link
Collaborator

bennofs commented Oct 27, 2015

Fixed the remaining errors on Windows and the curl issue. Should be good to merge now.

@bennofs

This comment has been minimized.

Copy link
Collaborator

bennofs commented Nov 3, 2015

ping @23Skidoo (hope pinging is OK)

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Nov 3, 2015

Sorry for the delay. Will take a look now.

@23Skidoo

This comment has been minimized.

There's a security issue with passing the password on the command line, see haskell#2764 (comment).

@23Skidoo

This comment has been minimized.

Ditto.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Nov 3, 2015

LGTM except that small security issue. Also would be nice to see a changelog entry. Thanks again for working on this!

bennofs added some commits Nov 4, 2015

HttpUtils: don't pass passwords as plain arguments
Passing passwords via command line arguments is insecure: anyone who
is able to read the process list on the system can read the passwords
as well.

For curl, we pass the password via stdin by using the --config option,
which allows us to pass arbitrary additional options via stdin.

Unfortunately, wget's --config option does not support - for stdin. So
we instead use the --input-file option to pass an URI with the password
via stdin.
@bennofs

This comment has been minimized.

Copy link
Collaborator

bennofs commented Nov 4, 2015

@23Skidoo changelog entry added, security issues fixed. I'm not quite sure here about the powershell situation regarding password security: the password is in cleartext in the generated script, so anyone who can read the script also has access to the password. Are the temporary files generated by cabal only readable by the current user?

bennofs added some commits Nov 4, 2015

HttpUtils: pass powershell script via stdin
The powershell script contains the password, so writing it to a file
is a bit unsafe. We know pass it via stdin.
This commit also improves the error message for network failures a
little bit.

23Skidoo added a commit that referenced this pull request Nov 5, 2015

Merge pull request #2890 from bennofs/upload-doc
Implement `cabal upload --doc`

@23Skidoo 23Skidoo merged commit 945dbc4 into haskell:master Nov 5, 2015

1 check passed

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

This comment has been minimized.

Copy link
Member

23Skidoo commented Nov 5, 2015

Merged, thanks!

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