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

geckodriver 0.26.0 #45906

Closed
wants to merge 1 commit into from
Closed

geckodriver 0.26.0 #45906

wants to merge 1 commit into from

Conversation

chrmoritz
Copy link
Contributor

@chrmoritz chrmoritz commented Oct 28, 2019

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Continuation of #45217. See #45217 (comment) for why the switch to the full mozilla-central source code is necessary here. Also Refs #45839 for adding --locked to cargo install.

@andreastt I'm seeing this warning during cargo install:

warning: Patch `coreaudio-sys v0.2.2 (/private/tmp/geckodriver-20191028-33664-4v1ucp/mozilla-central-e9783a644016aa9b317887076618425586730d73/third_party/rust/coreaudio-sys)` was not used in the crate graph.
Patch `cranelift-codegen v0.44.0 (https://github.com/CraneStation/Cranelift?rev=182414f15c18538dfebbe040469ec8001e93ecc5#182414f1)` was not used in the crate graph.
Patch `cranelift-wasm v0.44.0 (https://github.com/CraneStation/Cranelift?rev=182414f15c18538dfebbe040469ec8001e93ecc5#182414f1)` was not used in the crate graph.
Patch `libudev-sys v0.1.3 (/private/tmp/geckodriver-20191028-33664-4v1ucp/mozilla-central-e9783a644016aa9b317887076618425586730d73/dom/webauthn/libudev-sys)` was not used in the crate graph.
Patch `packed_simd v0.3.3 (https://github.com/hsivonen/packed_simd?branch=rust_1_32#3541e381)` was not used in the crate graph.
Check that the patched package version and available features are compatible
with the dependency requirements. If the patch has a different version from
what is locked in the Cargo.lock file, run `cargo update` to use the new
version. This may also occur with an optional dependency that is not enabled.

Is this something we should worry about or can this be safely ignored.

ToDo:

  • test this more carefully

@andreastt
Copy link

That warning doesn’t look harmful, but you also shouldn’t have to invoke cargo directly when building geckodriver.

./mach build testing/geckodriver will ensure cargo gets invoked the right way and this will produce a binary under dist/bin/ in the object directory.

The object directory should be named something like x86_64-apple-darwin, except for macOS.

I hope this helps!

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Oct 28, 2019

@andreastt: Unfortunately it looks like we have to stick with calling cargo install directly, because your ./mach build tooling doesn't seem to support macOS Catalina at all yet. First you'll get:

ERROR: SDK version "10.15" is unsupported. Please downgrade to version 10.14.

and if you try to override the sdk_max_version and set it to 10.15 you will get:

Could not find installed Xcode Command Line Tools; run `xcode-select --install` and follow [...]

even though they are installed correctly (I think there were some changes with Catalina there).

On the other hand calling cargo install directly works just fine and also has the advantage that is installs the binary in the correct prefix for us without having to do it manually.

@chrmoritz chrmoritz marked this pull request as ready for review October 28, 2019 23:08
@fxcoudert fxcoudert closed this in b39ef61 Oct 30, 2019
url "https://github.com/mozilla/geckodriver/archive/v0.25.0.tar.gz"
sha256 "9ba9b1be1a2e47ddd11216ce863903853975a4805e72b9ed5da8bcbcaebbcea9"
# Get the commit id for stable releases from https://github.com/mozilla/geckodriver/releases
url "https://hg.mozilla.org/mozilla-central/archive/e9783a644016aa9b317887076618425586730d73.tar.gz"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This url returns an error

curl: (22) The requested URL returned error: 403 Archive type not allowed: gz
Error: An exception occurred within a child process:
  DownloadError: Failed to download resource "geckodriver"
Download failed: https://hg.mozilla.org/mozilla-central/archive/e9783a644016aa9b317887076618425586730d73.tar.gz

Copy link
Contributor Author

@chrmoritz chrmoritz Nov 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reporting this. Looks like the only allowed archive type is now .zip. Working on a fix: #46746.

Copy link

@mastef mastef Nov 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np @chrmoritz was a bit frustrating after the rust dependency compiling for 2-3 hours, and then this step failing... would be also great to have just rustup being called instead of having to compile rust from source - but no idea where that's happening found it #5263

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you on an unsupported version of macOS (<= Sierra)? If not, you could also use homebrew's bottles for rust (and geckodriver) if you are considering consuming upstreams rust binaries via rustup?

@chrmoritz chrmoritz mentioned this pull request Nov 14, 2019
5 tasks
@lock lock bot added the outdated PR was locked due to age label Jan 2, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants