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

deno v0.3.0 (new formula) #35645

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
@hayd
Copy link

hayd commented Jan 3, 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>)?

http://github.com/denoland/deno

A binary-only version of this PR was closed: #35590

Notes:

  • It seems like git checkout, rather than zip, is required since deno uses submodules.
  • There is a single test.py failure at the moment, hopefully we can debug this. It's a bug in that test, will be fixed soon (when testing in a directory not named "deno").
  • There are some linker warnings, I'm not sure if these are a concern ld: warning: object file ... was built for newer OSX version (10.14) than being linked (10.9). I'm told these are fine.
  • Is there a way to specify the rust version? depends_on "rust@1.30.0" didn't work. I don't think rust version is important.

@hayd hayd referenced this pull request Jan 3, 2019

Closed

Install on macOs using homebrew? #8

@hayd

This comment has been minimized.

Copy link
Author

hayd commented Jan 3, 2019

I have to include test.py in the install block, if I try in the test block it gives this error:

16:37:47 ==> python tools/test.py
16:37:47 python: can't open file 'tools/test.py': [Errno 2] No such file or directory

Does this seem reasonable to include in the install block and have a simpler check of the binary?

@lembacon lembacon added the new formula label Jan 3, 2019

@hayd

This comment has been minimized.

Copy link
Author

hayd commented Jan 3, 2019

I've commented out test.py in the install step for now (due to a test bug), but it would be great to confirm whether this would be the correct place for it.

There's this remaining error:

23:34:28 ==> brew bottle --merge --write --no-commit ./deno--0.2.5.high_sierra.bottle.json

23:34:29 ==> FAILED
23:34:29 Error: Bottle block addition failed!
23:34:29 ==> deno
23:34:29   bottle do
23:34:29     cellar :any_skip_relocation
23:34:29     sha256 "1440e23219d017e7fbdd242321a17394a7d11e4737d003eca46329f58298bf80" => :high_sierra
23:34:29   end

which I am not sure how to address.

Edit: The error is fixed by adding an empty bottle block, which means the code takes a different path. This may be a (minor) bug in Homebrew.

@hayd hayd force-pushed the hayd:deno2 branch 2 times, most recently from 094dc0a to b5a5b6b Jan 4, 2019

@hayd

This comment has been minimized.

Copy link
Author

hayd commented Jan 4, 2019

The build is passing now.

Show resolved Hide resolved Formula/deno.rb Outdated
Show resolved Hide resolved Formula/deno.rb Outdated
Show resolved Hide resolved Formula/deno.rb Outdated
Show resolved Hide resolved Formula/deno.rb Outdated
Show resolved Hide resolved Formula/deno.rb Outdated
Show resolved Hide resolved Formula/deno.rb Outdated
@javian

This comment has been minimized.

Copy link
Member

javian commented Jan 4, 2019

I was about to tag this as a rust project but there seems to be so many components involved and most of the code is written in typescript according th github. Can you provide some clarity on this topic ?

@hayd

This comment has been minimized.

Copy link
Author

hayd commented Jan 4, 2019

Is it possible to tag as both typescript and rust?
The "front-end" is typescript (js spec definitions are here, and sends messages to rust).
The "Back-end" is rust (wires up the event loop, fs access etc).
There's also a small "middle-end" is C++ binding to v8 (which is why the build is a little complicated).

https://github.com/denoland/deno/blob/master/Docs.md#internals
https://denolib.gitbook.io/guide/codebase-basics/infrastructure

Edit2: deno is similar to node which is written mostly in javascript but the back-end is written in C++, so tag however you would tag node?

Edit1: Thanks for the review!

@BrewTestBot

This comment has been minimized.

Copy link
Contributor

BrewTestBot commented Jan 8, 2019

  • Dependency 'llvm' may be unnecessary as it is provided by macOS; try to build this formula without it.

@hayd hayd force-pushed the hayd:deno2 branch from 42e4888 to 6db2c50 Jan 8, 2019

@hayd hayd changed the title deno v0.2.5 (new formula) deno v0.2.6 (new formula) Jan 8, 2019

@hayd

This comment has been minimized.

Copy link
Author

hayd commented Jan 8, 2019

@javian I think I've addresses/answered the feedback.

I've also bumped the version to 0.2.6.

@hayd hayd referenced this pull request Jan 9, 2019

Open

Install deno with homebrew #1486

@sholladay

This comment has been minimized.

Copy link

sholladay commented Jan 21, 2019

Friendly ping. :)

I'm very excited about Deno and adding it to Homebrew. Is there anything I can do to help it along?

@hayd hayd force-pushed the hayd:deno2 branch from 6db2c50 to f34b97e Jan 28, 2019

@hayd hayd changed the title deno v0.2.6 (new formula) deno v0.2.8 (new formula) Jan 28, 2019

@hayd hayd force-pushed the hayd:deno2 branch from f34b97e to a54f0c3 Jan 30, 2019

Show resolved Hide resolved Formula/deno.rb Outdated

@hayd hayd force-pushed the hayd:deno2 branch from a54f0c3 to 3583fc6 Feb 9, 2019

@hayd hayd changed the title deno v0.2.8 (new formula) deno v0.2.10 (new formula) Feb 9, 2019

@hayd

This comment has been minimized.

Copy link
Author

hayd commented Feb 9, 2019

There is a new error here (this had been passing before I rebased):

dyld: Library not loaded: /usr/local/opt/openssl@1.1/lib/libssl.1.1.dylib
  Referenced from: /private/tmp/deno-20190209-96380-iis3wz/prebuilt/mac/sccache
  Reason: image not found

Do I need to add openssl as dependency?

@javian

This comment has been minimized.

Copy link
Member

javian commented Feb 10, 2019

@hayd you added the wrong openssl version though it should be "openssl@1.1" since that's what it is asking for

@BrewTestBot

This comment has been minimized.

Copy link
Contributor

BrewTestBot commented Feb 10, 2019

  • Dependency 'openssl@1.1' may be unnecessary as it is provided by macOS; try to build this formula without it.
Show resolved Hide resolved Formula/deno.rb Outdated
@ry

This comment has been minimized.

Copy link

ry commented Feb 13, 2019

Any updates on this? Are there anythings deno can do to help it get in?

Show resolved Hide resolved Formula/deno.rb Outdated
@ry

This comment has been minimized.

Copy link

ry commented Feb 13, 2019

@javian

I was about to tag this as a rust project but there seems to be so many components involved and most of the code is written in typescript according th github. Can you provide some clarity on this topic ?

Deno straddles the Rust world and the Chromium world. Ultimately it's more of a Chromium style project - because it make heavy usage of V8 (which is Chromium's VM for executing JavaScript). Thus it uses Chromium's build system (GN). Deno masquerades as a typical Rust crate, and cargo build can be used to invoke the Chromium build system (GN). When using "cargo build", the source code for dependent crates will be installed in a cargo controlled directory, but when using "./tools/build.py" we build from a vendor directory. For ourselves, we use tools/build.py (GN) to develop Deno, because it's faster at linking than cargo is. However for brew, I think it probably makes more sense to use the "cargo build" method. In either case, there are required build dependencies like Python and Node. We've recently clean up the documentation for building from source - maybe that helps.

@ry

This comment has been minimized.

Copy link

ry commented Feb 21, 2019

Neither sccache nor hyperfine are technically required to build (sccache is very useful if you don't to spend hours building... but that requires a place to keep the cache. Does Homebrew have that functionality? Where does homebrew do the builds?)

There are a number of other binaries that we use to build, here is a list:

  • We download and use a specific version of clang from Google (See third_party/v8/tools/clang/scripts/update.py)
  • We download binary version of the build tools ninja and gn (See third_party/depot_tools/download_from_google_storage.py)

These steps are not unique to Deno - any Chromium project would have the same requirements.

I've had a look at the formula for V8. It also uses depot_tools so it does make the above binary downloads.

Downloading binaries is not acceptable for Homebrew core.

Should v8.rb be removed?

@fxcoudert

This comment has been minimized.

Copy link
Member

fxcoudert commented Feb 21, 2019

  • Is there no way to ask it to use system ninja, sccache, etc.?
  • Yes, v8 is an exception, for many reasons… one of them being that it's widely used. The question is thus whether we want to extend that exception to deno, or if it should be a binary cask.

Let's see how other maintainers feel about it.

@ry

This comment has been minimized.

Copy link

ry commented Feb 21, 2019

We can certainly use depot_tool checkout steps as V8 does in the Deno formula rather than using our checkout of depot_tools, if that makes people feel more comfortable...
But note that there will still be binaries being downloaded - as is done in v8.rb - just that we can be sure that they're coming from google instead of randos like us. I'd like to get a green light that that's okay before pursing it.

Also looks like /Formula/sync_gateway.rb uses depot_tools too.

sccache and hyperfine can be ignored. I can land something upstream to make that ignoring easier for homebrew.

@claui

This comment has been minimized.

Copy link
Member

claui commented Feb 21, 2019

But note that there will still be binaries being downloaded

At build time?

Are you referring to binaries downloaded by third_party.py?

@ry

This comment has been minimized.

Copy link

ry commented Feb 21, 2019

@claui v8 (via depot_tools) downloads various binaries (clang, gn, ninja but maybe some others too)

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 22, 2019

  • We download and use a specific version of clang from Google (See third_party/v8/tools/clang/scripts/update.py)
  • We download binary version of the build tools ninja and gn (See third_party/depot_tools/download_from_google_storage.py)

I'm afraid downloading and using binaries (particularly for things already provided by Xcode or Homebrew) during the build process is not acceptable to Homebrew nor would it be to many other package managers. If there's a security update required to any of those tools then this would make it very difficult for us to update them.

Ideally I'd like to see v8 updated to remove this stuff too. It may be interesting to look at how Debian packages v8: https://packages.debian.org/search?keywords=libv8

@linguofeng

This comment has been minimized.

Copy link

linguofeng commented Feb 28, 2019

Any updates?

@fxcoudert

This comment has been minimized.

Copy link
Member

fxcoudert commented Feb 28, 2019

@hayd can you make deno use the v8 from Homebrew?

@ry

This comment has been minimized.

Copy link

ry commented Feb 28, 2019

Making this work with homebrew’s V8 is going to be difficult. Deno requires bleeding edge fixes in V8 and we don’t have the ability (nor want) to link to an external version.

However the getting this to not make downloads is not hard. I suggest:

  1. Get deno’s setup.py to take something like a —no-binary-download argument - which prevents sccache and other binaries from being downloaded.
  2. Use the depot_tools setup that the V8 formula uses to get gn and ninja.
@linguofeng

This comment has been minimized.

Copy link

linguofeng commented Mar 12, 2019

Any updates?

@claui

This comment has been minimized.

Copy link
Member

claui commented Mar 12, 2019

Ideally I'd like to see v8 updated to remove this stuff too. It may be interesting to look at how Debian packages v8: https://packages.debian.org/search?keywords=libv8

Looks like I can’t Debian today. 😄 I ended up at https://packages.debian.org/source/stretch/libv8-3.14 but that’s a dead end somehow. Clicking on the source repositories for stretch gives me nothing but 404s.

A Google search didn’t help either. (Last commit in 2011, no traces of either gn nor ninja.)

@MikeMcQuaid What did I miss?

Anyone else who is better at Debianing than I am?

@ry

This comment has been minimized.

Copy link

ry commented Mar 12, 2019

Re debian - it's a much better experience for users, and easier for us, if Debian does not try to split V8 out of Deno (by that I mean to dynamically link). We depend on the very latest features and bug fixes, we will not back port to older versions of V8, and we reserve the right to float patches on top of V8.

It's an integral part of Deno, and should be statically linked into the binary, as our build system provides.

@claui

This comment has been minimized.

Copy link
Member

claui commented Mar 12, 2019

It's an integral part of Deno, and should be statically linked into the binary

@ry Point taken.


Get deno’s setup.py to take something like a —no-binary-download argument

Just to be perfectly clear, such an option would have setup.py skip those downloads? But some later step in Deno’s build system would still somehow require the gn and ninja binaries being there?

Use the depot_tools setup that the V8 formula uses to get gn and ninja.

How would that not be a binary download?

@ry

This comment has been minimized.

Copy link

ry commented Mar 12, 2019

Just to be perfectly clear, such an option would have setup.py skip those downloads? But some later step in Deno’s build system would still somehow require the gn and ninja binaries being there?

Chromium and V8 and Deno all build with a system called "depot_tools". Deno includes depot_tools so our users don't have to install it separately. So —no-binary-download would just skip this bit here:
https://github.com/denoland/deno/blob/ad21be837036574154e8aa69b06001a1eb380b2b/tools/setup.py#L18-L21
Then, follow how V8 does it:
https://github.com/Homebrew/homebrew-core/blob/master/Formula/v8.rb#L5-L6

system "gclient", "root"
system "gclient", "config", "--spec", <<~EOS
solutions = [
{
"url": "https://chromium.googlesource.com/v8/v8.git",
"managed": False,
"name": "v8",
"deps_file": "DEPS",
"custom_deps": {},
},
]
target_os = [ "mac" ]
target_os_only = True

Turns out we don't even need --no-binary-download ... once we have depot tools calling setup.py is redundant.

ry added a commit to ry/deno that referenced this pull request Mar 12, 2019

@ry

This comment has been minimized.

Copy link

ry commented Mar 12, 2019

I've taken a crack at it here denoland/deno#1917
I don't know how to test this... Is there a good way to test a homebrew formula in CI?

hayd added some commits Jan 3, 2019

deno v0.2.10 (new formula)
Address feedback
Revert "Try not removing symlinks"
This reverts commit 44db44d.

@hayd hayd force-pushed the hayd:deno2 branch from 0c0b40d to f3d6d65 Mar 12, 2019

@ry

This comment has been minimized.

Copy link

ry commented Mar 14, 2019

essentially deno needs only two tools to bootstrap the build: gn and ninja.
It seems there's a formula for ninja already https://formulae.brew.sh/formula/ninja
If we had a formula for gn, this would be much easier https://gn.googlesource.com/gn/
Having a formula for gn would potentially allow V8's formula to avoid depot_tools too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.