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

Remove check for ssl_enabled for ConfigBuilder #22535

Closed
wants to merge 1 commit into from

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Dec 22, 2018

Fixes #22252. This is a rebased version of #22253.


This change is Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 22, 2018
@servo-wpt-sync
Copy link
Collaborator

Error syncing changes upstream. Logs saved in error-snapshot-1545509473194.

@KiChjang
Copy link
Contributor Author

Oh crud, something's wrong with the WPT sync. cc @jdm

@jdm
Copy link
Member

jdm commented Dec 22, 2018

In this case there's probably a merge conflict because we're 5 days behind.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #22536) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Dec 23, 2018
@servo-wpt-sync
Copy link
Collaborator

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#14662.

@KiChjang
Copy link
Contributor Author

KiChjang commented Jan 6, 2019

r? @jdm

This should fix the problems I'm encountering on Windows. Essentially, the SSL config getter should ALWAYS return a record, regardless of whether or not SSL was enabled.

@highfive highfive assigned jdm and unassigned ferjm Jan 6, 2019
@KiChjang
Copy link
Contributor Author

KiChjang commented Jan 6, 2019

Should probably cc @jgraham as well.

@jdm
Copy link
Member

jdm commented Jan 15, 2019

@KiChjang What is the effect of these changes on your local build?

@jgraham
Copy link
Contributor

jgraham commented Jan 15, 2019

I think this is probably OK.

@jdm
Copy link
Member

jdm commented Jan 15, 2019

Actually, I think I see how the SSL stuff works and why it's not working on Windows. Rather than making this change, I propose the following:

  • in the build_env method in the msvc branch, add the openssl package's binary directory to the PATH

Since wptcommandline's --openssl-binary defaults to openssl, this will allow windows and non-windows platforms to take the same code path, and should allow running tests on Windows.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 15, 2019
@jdm
Copy link
Member

jdm commented Jan 15, 2019

The other reasonable solution here would be modifying our WPT initialization to use the pregenerated certs from tests/wpt/web-platform-tests/tools/certs by setting the --ca-cert-path, --host-cert-path, and --host-key-path arguments appropriately.

@KiChjang
Copy link
Contributor Author

@jdm Hrm, currently we do not require windows users to install openssl when configuring their system to build servo. Should we add that to the README.md? Or did you mean the python package that is called openssl?

@jdm
Copy link
Member

jdm commented Jan 15, 2019

We automatically download openssl as part of the windows bootstrapping process: https://github.com/servo/servo/blob/master/python/servo/packages.py#L10

@KiChjang
Copy link
Contributor Author

So, I found out that the --ssl-type command line argument defaults to none, which causes the WPT runner to not use any SSL config at all, which means even if I did set the openssl binary path correctly for windows, it still wouldn't use any SSL config at all. @jdm Should we just default ssl_type to openssl altogether? Or would pregenerated make more sense here?

@KiChjang
Copy link
Contributor Author

I'm also seeing that the code behaves extremely weirdly where defaults are applied after the CLI kwargs are parsed, and not during parsing. This leads to surprising behavior where if the ssl_type is changed in the middle of some code, it would somehow Just Work and use the correct openssl binary/certs, even though the corresponding CLI args aren't passed into wptrunner.

@jdm
Copy link
Member

jdm commented Mar 20, 2019

If I'm correct, your previous two comments refer to these bits of code:

This is why I suggested adding the openssl binary to the path on Windows. Really, though, everything will be easier if we set up our ca_cert_path/host_cert_path/host_key_path defaults to point at the pregenerated certs on all platforms. It should be possible to do that in set_defaults, which calls check_args.

@jdm
Copy link
Member

jdm commented Mar 29, 2019

Closing in favour of the description in #23133.

@jdm jdm closed this Mar 29, 2019
@servo-wpt-sync
Copy link
Collaborator

Error syncing changes upstream. Logs saved in error-snapshot-1553873359641.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-needs-rebase There are merge conflict errors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants