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.

Sign up for GitHub

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

Jump to bottom

chakra.rb: update ChakraCore to release 1.5 #13947

Closed
wants to merge 1 commit into from
Closed

chakra.rb: update ChakraCore to release 1.5 #13947

wants to merge 1 commit into from

Conversation

obastemur
Copy link
Contributor

@obastemur obastemur commented May 25, 2017 •
edited
Loading

Also;

  • Enable LTO build
  • Faster builds with j2 -j
  • Make ICU dependency a requirement. Better experience and compatibility.

ilovezfs
ilovezfs reviewed May 26, 2017
View reviewed changes
Formula/chakra.rb Outdated
system "./build.sh", *args

bin.install "BuildLinux/Release/ch" => "chakra"
system "./build.sh", "--lto-thin", "--static", "--embed-icu", "-j=2", "-y"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any objection to -j=#{ENV.make_jobs}?

@obastemur
Copy link
Contributor Author

@ilovezfs Thanks for the review. Updated the commit. Please let me know if anything else needs touch.

ilovezfs
ilovezfs reviewed May 26, 2017
View reviewed changes
Formula/chakra.rb Outdated
system "./build.sh", *args

bin.install "BuildLinux/Release/ch" => "chakra"
system "./build.sh", "--lto-thin", "--static", "--embed-icu", "-j=#{ENV.make_jobs}", "-y"
Copy link
Contributor

Choose a reason for hiding this comment

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

This script will download ICU-LIB from
http://source.icu-project.org/repos/icu/icu/tags/release-57-1

Is the checksum being verified? If not, can we vendor the resource instead in a resource block? Does it not work with our icu4c formula (58.2) and soon to be 59.1 (#12461) yet?

Copy link
Contributor

@ilovezfs ilovezfs May 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like it's using svn and an insecure http url, so I guess we'll need to use a resource block or the brew formula.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the checksum being verified? If not, can we vendor the resource instead in a resource block? Does it not work with our icu4c formula (58.2) and soon to be 59.1 (#12461) yet?

The problem with icu4c formula is, it couldn't find the dylib on CI. however it was working okay on my machine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated once again to brew formula. Let's see. If it doesn't work, I will update to no-icu again and next release we may use it from secure url.

Copy link
Contributor

Choose a reason for hiding this comment

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

@obastemur That's because you don't want => :build which means the dependency is build-time only. To protect against erroneous => :build we uninstall them before running the test block!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, instead of :build it should has :run ?

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't want => :run either. Just

depends_on "icu4c"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem. I'll check if it build with v59 locally.

@obastemur
chakra.rb: update ChakraCore to release 1.5
20183a6 
Also;
- Enable LTO build
- Faster builds with `-j`
- Make ICU dependency a requirement. Better experience and compatibility.
@ilovezfs
Copy link
Contributor

It builds fine with icu4c 59.1 too:

==> Summary
🍺  /usr/local/Cellar/chakra/1.5.0: 5 files, 14.7MB, built in 5 minutes 39 seconds
iMac-TMP:homebrew-core joe$ brew test chakra
Testing chakra
==> Using the sandbox
==> /usr/local/Cellar/chakra/1.5.0/bin/chakra test.js
iMac-TMP:homebrew-core joe$ brew linkage chakra
System libraries:
  /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
  /System/Library/Frameworks/Security.framework/Versions/A/Security
  /usr/lib/libSystem.B.dylib
  /usr/lib/libc++.1.dylib
Homebrew libraries:
  /usr/local/opt/icu4c/lib/libicudata.59.1.dylib (icu4c)
  /usr/local/opt/icu4c/lib/libicui18n.59.dylib (icu4c)
  /usr/local/opt/icu4c/lib/libicuuc.59.dylib (icu4c)

So #12461 won't break it.

@ilovezfs ilovezfs closed this in f5c9354 May 26, 2017
@ilovezfs
Copy link
Contributor

@obastemur Thanks for the upgrade. Shipped!

Just for your future reference:

[joe:/usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula] master+ 6s ± brew pull --bottle https://github.com/Homebrew/homebrew-core/pull/13947
==> Fetching patch 
Patch: https://github.com/Homebrew/homebrew-core/pull/13947.patch
==> Applying patch
Applying: chakra.rb: update ChakraCore to release 1.5
==> Patch closes issue #13947
Warning: Nonstandard bump subject: chakra.rb: update ChakraCore to release 1.5
Warning: Subject should be: chakra 1.5.0

I went ahead and adjusted the commit subject for you while pulling :)

@obastemur
Copy link
Contributor Author

Great! Thanks again

@ilovezfs
Copy link
Contributor

You're welcome!

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ilovezfs ilovezfs ilovezfs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@obastemur @ilovezfs