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 1.7.0 #66920

Closed
wants to merge 2 commits into from
Closed

deno 1.7.0 #66920

wants to merge 2 commits into from

Conversation

chimurai
Copy link
Contributor

Created with brew bump-formula-pr.

@BrewTestBot BrewTestBot added the rust Rust use is a significant feature of the PR or issue label Dec 14, 2020
@carlocab
Copy link
Member

carlocab commented Dec 14, 2020

Is it possible for this not to be merged until after #66285?

The CI run for that PR takes nearly two full days to complete, and this PR will create a merge conflict. If I have to rebase the Rust PR against master again, then that will cause another two-day delay.

@chimurai
Copy link
Contributor Author

Removed the patch in PR, which is included in deno 1.6.1

Got no problems to wait. Up to mods to respect your request @carlocab 👍

@carlocab
Copy link
Member

It would help if you marked this PR as a draft in the meantime, but I hope a maintainer sees my previous comment before approving.

@chimurai chimurai marked this pull request as draft December 14, 2020 23:15
@chimurai chimurai marked this pull request as ready for review December 15, 2020 14:23
@chimurai
Copy link
Contributor Author

Thanks for adding do not merge label @bayandin

I removed the draft. @carlocab, can you give a ping when this can be merged?

@carlocab
Copy link
Member

Will do. Thanks!

@carlocab carlocab mentioned this pull request Dec 16, 2020
5 tasks
@JounQin
Copy link
Contributor

JounQin commented Dec 16, 2020

Can depends_on :macos be removed in favor of denoland/deno#7779

@gromgit
Copy link
Member

gromgit commented Dec 16, 2020

Can depends_on :macos be removed in favor of denoland/deno#7779

From that issue:

Note: Building with python3 works provided you don't build V8. 😎

This formula does build V8, if I'm reading it correctly. I suggest wrapping up this PR first, then you can open a separate one to do the migration.

@bayandin
Copy link
Member

This formula does build V8, if I'm reading it correctly. I suggest wrapping up this PR first, then you can open a separate one to do the migration.

The formula itself doesn't, but deno uses rusty_v8 which could build it.

@carlocab
Copy link
Member

Rust PR merged. Thanks for waiting, @chimurai.

@bayandin bayandin added CI-requeued PR has been re-added to the queue and removed do not merge labels Dec 16, 2020
chenrui333
chenrui333 previously approved these changes Dec 16, 2020
@chenrui333
Copy link
Member

Thanks all, merging! :)

@BrewTestBot
Copy link
Member

:shipit: @chenrui333 has triggered a merge.

@BrewTestBot
Copy link
Member

⚠️ @chenrui333 bottle publish failed.

@carlocab
Copy link
Member

carlocab commented Dec 18, 2020

Seems to succeed on 10.14 but not on 10.15 and 11.0. Do we know if deno is compatible with Xcode 12.2? Looked into it; it should be.

@Rylan12
Copy link
Member

Rylan12 commented Dec 18, 2020

Sorry, I think this will have a conflict now that #67043 is merged. I'll resolve and update here.

@fxcoudert fxcoudert removed CI-requeued PR has been re-added to the queue ready to merge PR can be merged once CI is green labels Dec 18, 2020
@chenrui333 chenrui333 changed the title deno 1.6.1 deno 1.6.2 Dec 22, 2020
@carlocab
Copy link
Member

This is weird:

HOMEBREW_SDKROOT: /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk

@bayandin
Copy link
Member

Shall we try to build if for Apple Silicon as well next time?

@carlocab carlocab added the CI-force-arm [DEPRECATED] Don't pass --skip-unbottled-arm to brew test-bot. label Jan 21, 2021
@carlocab
Copy link
Member

Yes. I'd love ideas on how to fix the Catalina build tho.

@chrmoritz
Copy link
Contributor

chrmoritz commented Jan 21, 2021

Ok, better than last time, where the Mojave runner failed too. The failure is totally uninformative as usual though.

The error is in the gn / rusty_v8 part and at about 60% through the log:

for 10.14

[207/1259] LIBTOOL-STATIC obj/v8/libv8_libplatform.a
  FAILED: obj/v8/libv8_libplatform.a 
  rm -f obj/v8/libv8_libplatform.a && TOOL_VERSION=1611011547 python ../../../../../../../Users/brew/Library/Caches/Homebrew/cargo_cache/registry/src/github.com-1ecc6299db9ec823/rusty_v8-0.16.0/build/toolchain/mac/filter_libtool.py /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool -static  -o obj/v8/libv8_libplatform.a -filelist obj/v8/libv8_libplatform.a.rsp
  fatal error: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: can't open file list file: obj/v8/libv8_libplatform.a.rsp (No such file or directory)

for 10.15 (old run):

[1253/1260] ACTION //v8:run_mksnapshot_default(//build/toolchain/mac:clang_x64)
  FAILED: gen/v8/embedded.S gen/v8/snapshot.cc 
  python ../../../../../../../Users/brew/Library/Caches/Homebrew/cargo_cache/registry/src/github.com-1ecc6299db9ec823/rusty_v8-0.16.0/v8/tools/run.py ./mksnapshot --turbo_instruction_scheduling --target_os=mac --target_arch=x64 --embedded_src gen/v8/embedded.S --embedded_variant Default --random-seed 314159265 --startup_src gen/v8/snapshot.cc --no-native-code-counters
  Return code is -9

for 10.15 (new run):

[183/1260] LIBTOOL-STATIC obj/v8/libv8_libbase.a
  FAILED: obj/v8/libv8_libbase.a 
  rm -f obj/v8/libv8_libbase.a && TOOL_VERSION=1611011547 python ../../../../../../../Users/brew/Library/Caches/Homebrew/cargo_cache/registry/src/github.com-1ecc6299db9ec823/rusty_v8-0.16.0/build/toolchain/mac/filter_libtool.py /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool -static  -o obj/v8/libv8_libbase.a -filelist obj/v8/libv8_libbase.a.rsp
  fatal error: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: can't open file list file: obj/v8/libv8_libbase.a.rsp (No such file or directory)

You have to scroll up through a lot of rust output to get to the actual gn output with the errors.

@carlocab
Copy link
Member

I think those come from my having removed the Xcode build dependency. I couldn't find any documentation about it being needed, whether in Deno's or rusty_v8's docs, so I thought it would be safe to remove.

@chrmoritz
Copy link
Contributor

chrmoritz commented Jan 21, 2021

I think those come from my having removed the Xcode build dependency. I couldn't find any documentation about it being needed, whether in Deno's or rusty_v8's docs, so I thought it would be safe to remove.

I'm pretty sure the V8 build has some hard coded requirement on XCode in it's build system and doesn't support CLT only systems.
On the other hand I'm not sure if removing that requirement is relevant for the error on systems where XCode is at least still installed and accessible.

@carlocab
Copy link
Member

carlocab commented Jan 21, 2021

I'm pretty sure the V8 build has some hard coded requirement on XCode in it's build system and doesn't support CLT only systems.

I believe that; it's just not mentioned in their docs. (They even explicitly only require CLT, and mention that you don't need it if you have Xcode.)

On the other hand I'm not sure if removing that requirement is relevant for the error on systems where XCode is at least still installed and accessible.

Superenv changes environment variables depending on the declared dependencies though. For example, without the Xcode build dependency, HOMEBREW_SDKROOT is

HOMEBREW_SDKROOT: /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk

With it, it looks something like

HOMEBREW_SDKROOT: /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk

These environment variables then affect how the clang shim calls clang.

@chrmoritz
Copy link
Contributor

chrmoritz commented Jan 21, 2021

I believe that; it's just not mentioned in their docs. (They even explicitly only require CLT, and mention that you don't need it if you have Xcode.)

But this doc are for people who use their prebuilt (rusty_)V8 binary and not accurate if you build all dependencies, including (rusty_)V8 from source.

Superenv changes environment variables depending on the declared dependencies though.

Okay, that might be the cause then. It's still strange that the build failures aren't deterministic and seem to fail randomly in different steps. But maybe the CLT HOMEBREW_SDKROOT is causing some issues with V8 build system, which hard codes XCode somewhere else. 🤷

@carlocab
Copy link
Member

Ok, looks like 10.15 failed in 77e4ad8 long before it decided to stop: https://github.com/Homebrew/homebrew-core/runs/1741214532#step:7:6447

I wonder if there's some sort of FAIL_FAST option...

@carlocab
Copy link
Member

But this doc are for people who use their prebuilt (rusty_)V8 binary and not accurate if you build all dependencies, including (rusty_)V8 from source.

Yup, I had a look at the rusty_V8 docs too. No mention of Xcode. Though, I suppose this isn't as carefully documented if they just fully intend for you to download a binary.

@carlocab
Copy link
Member

It's still strange that the build failures aren't deterministic and seem to fail randomly in different steps.

Yes. See #66920 (comment)

@chrmoritz
Copy link
Contributor

But this doc are for people who use their prebuilt (rusty_)V8 binary and not accurate if you build all dependencies, including (rusty_)V8 from source.

Yup, I had a look at the rusty_V8 docs too. No mention of Xcode. Though, I suppose this isn't as carefully documented if they just fully intend for you to download a binary.

Yeah, the complicated why Chromiums tries to find the macOS SKD is here:

@carlocab
Copy link
Member

Talk about non-deterministic. Compare:

54c027e with CI logs at https://github.com/Homebrew/homebrew-core/runs/1745216015

vs

77e4ad8 with CI logs at https://github.com/Homebrew/homebrew-core/runs/1741214515

Is it just me, or are these two identical formulae with exactly opposing outcomes?

@carlocab
Copy link
Member

carlocab commented Jan 22, 2021

Ok, I patterned this latest commit after carlocab/homebrew-personal@0ab0b25 which builds successfully on GitHub runners (see https://github.com/carlocab/homebrew-personal/runs/1744816923).

Unfortunately, it also creates a runtime dependency on llvm, which isn't great. It seems that even if I don't remove llvm's libraries from HOMEBREW_LIBRARY_PATHS, there is no linkage:

❯ brew linkage deno@1.7
System libraries:
  /usr/lib/libSystem.B.dylib
  /usr/lib/libresolv.9.dylib

@carlocab
Copy link
Member

carlocab commented Jan 22, 2021

Ok, let's try to be at least a little systematic about this.

Formula State 11-arm 11 10.15 10.14 CI Logs
54c027e24f0ba91816ff906af17efe05d1b5d663 https://github.com/Homebrew/homebrew-core/actions/runs/502301161
77e4ad870875fb3666c56650622bafc01e7401e2 https://github.com/Homebrew/homebrew-core/actions/runs/500899264
10834714c2dc1339e4aec483113a64530dfde1dc https://github.com/Homebrew/homebrew-core/actions/runs/502946556
cce5be149e0966ea37b85a073e270b23d028a6f5 ✖️ https://github.com/Homebrew/homebrew-core/actions/runs/504751675
2060391a473356a9c44a8d78682a66f3420d2f9f ✖️ ✖️ https://github.com/Homebrew/homebrew-core/actions/runs/505233053
f91746c https://github.com/Homebrew/homebrew-core/actions/runs/505353591

chimurai and others added 2 commits January 23, 2021 06:33
- use Homebrew ninja
- download and build gn from source manually
- remove FORCE_MAC_SDK_MIN
- don't delete llvm from HOMEBREW_LIBRARY_PATHS
- use sccache to speed up build time
@carlocab
Copy link
Member

Ok, the current version of the formula builds on:

OS Version Commit CI Log
Mojave cce5be149e0966ea37b85a073e270b23d028a6f5 https://github.com/Homebrew/homebrew-core/actions/runs/504751675
Catalina carlocab/homebrew-personal@51f5900 https://github.com/carlocab/homebrew-personal/pull/32/checks?check_run_id=1752854844
Big Sur carlocab/homebrew-personal@51f5900 https://github.com/carlocab/homebrew-personal/pull/32/checks?check_run_id=1752854830
Big Sur-ARM cce5be149e0966ea37b85a073e270b23d028a6f5 https://github.com/Homebrew/homebrew-core/runs/1751756652?check_suite_focus=true

Just not all at the same time. I guess if we just keep re-running CI, we'll get lucky and find a run where all of them succeed? Otherwise, I may try and merge a run where ARM and Mojave build, and just try to bottle for Catalina and Big Sur separately. (Not sure if brew pr-pull will let me do that tho?)

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

I'm going to try to merge this and bottle for Big Sur separately.

@carlocab
Copy link
Member

@BrewTestBot
Copy link
Member

@carlocab bottle request for deno failed.

@chimurai chimurai deleted the bump-deno-1.6.1 branch January 23, 2021 13:49
@carlocab
Copy link
Member

Second bottle atttempt succeeded: 5c23efe

Thanks for the help all!

@carlocab
Copy link
Member

Non-deterministic builds reported at denoland/deno#9244.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-force-arm [DEPRECATED] Don't pass --skip-unbottled-arm to brew test-bot. no ARM bottle Formula has no ARM bottle rust Rust use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet