Add tools/build_third_party.py #328

kt3k · 2018-07-04T05:08:54Z

This PR tries to solve #312. I added tools/build_third_party.py, which runs gclient and yarn for updating gclient deps and npm deps.

I moved the place of node_modules from js/ to third_party/ (and made the symlink js/node_modules to point it) to enable separating third_party to a different repository and store all deps in it.

closes denoland#312

mreinstein · 2018-07-04T06:21:09Z

tools/build_third_party.py

+# --------------------
+# This script updates the third party dependencies of deno.
+#
+# - Get Depot Tools and make sure it's in your path.


It might be nice to print an error when depot tools aren't available/found

ditto for the yarn dep

Thanks for the comment! I agree. I'll try it.

Actually I prefer not adding extra code for those checks. run() will fail with a stack trace that can diagnose that issue. This script is not user facing.

qti3e · 2018-07-04T06:36:52Z

.gitignore

@@ -2,7 +2,7 @@
 /out/

 # npm deps
-node_modules
+/third_party/node_modules


please don't change this line - we might have multiple node_modules in near future

Thanks for the comment! I updated the line.

ry

Great - thank you!

I have a few comments...

ry · 2018-07-04T10:35:58Z

.travis.yml

@@ -28,7 +28,7 @@ install:
 - curl -sSf https://sh.rustup.rs | sh -s -- -y
 - export PATH=$HOME/.cargo/bin:$PATH
 - rustc --version
- - (cd js; yarn)
+ - (cd third_party; yarn)
 - (cd third_party; gclient sync -j2 --no-history)


You should be able to replace these lines with ./tools/build_third_party.py ?

If dropping -j2 doesn't cause a problem, then that should work. I'll try that on this PR.

Yes - that should be fine. (I think gclient is -j1 by default and I was just testing to see if -j2 made things faster - it doesn't - but it got left in)

ry · 2018-07-04T10:37:13Z

third_party/yarn.lock

@@ -54,12 +54,12 @@
  resolved "https://registry.yarnpkg.com/@types/long/-/long-3.0.32.tgz#f4e5af31e9e9b196d8e5fca8a5e2e20aa3d60b69"


Can you do this PR without upgrading the deps?

ry · 2018-07-04T10:38:13Z

tools/build_third_party.py

+# coding=utf-8
+#
+# build_third_party.py
+# --------------------


Lines 2-5 are unnecessary

ry · 2018-07-04T10:39:11Z

tools/build_third_party.py

+
+root_path = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
+third_party_path = os.path.join(root_path, "third_party")
+script_name = "build_third_party.py"


~~I think sync_third_party.py is a better name. What do you think?~~

build_third_party.py is better.

I agree. This script doesn't build things, but basically updates the deps.

@kt3k actually I changed my mind- it is only syncing now, but it will be doing more "build" type things later - like creating symlinks (see comment)

OK.

it will be doing more "build" type things later - like creating symlinks

That's why you called this build_* first.

ry · 2018-07-04T10:39:42Z

tools/build_third_party.py

+
+def main():
+    os.chdir(third_party_path)
+    print "cwd=" + third_party_path


Remove debug print

ry · 2018-07-04T10:40:08Z

tools/build_third_party.py

+    print "cwd=" + third_party_path
+    run(["gclient", "sync", "--no-history"])
+    run(["yarn"])
+    print "✨  Done (" + script_name + ")"


Remove emoji

ry · 2018-07-04T10:41:03Z

tools/build_third_party.py

+    print "✨  Done (" + script_name + ")"
+
+def run(args):
+    print "+" + " ".join(args)


Remove "+" +

ghost · 2018-07-04T11:54:05Z

I am waiting for your stabilized build scripts so that I can put them in #323. The essence is three files:

Since #323 doesn't touch .travis.yml, updating it would have no effect on CI. It might help you explore different build scripts for various Linux platforms that can be emulated in Docker. Travis CI is just one of the cases. We might want automated build on Azure/AWS/GCP etc. down the road. It is easy to write specific development/test/deployment work flows in docker compose. No need to be limited to the specific platform the library authors are working on.

ry · 2018-07-04T12:05:23Z

@ontouchstart Let's land this bit first. I will review #323 soon - sorry for the delay - but switching to docker is a lower priority and needs to be thought thru carefully.

ry · 2018-07-04T12:18:42Z

Eventually this script should also create //third_party/.gclient and //third_party/package.json symlinks during run-time. The reason is that when //third_party becomes a git submodule, it shouldn't have symlinks that reach outside of the repo.

Ideally we would be able to rm -rf third_party; ./tools/build_third_party.py and everything would be the same.

(PS we have a windows compatible symlink function in python here .. maybe we should start a python library of common utilities and that routine can be used here.)

ry · 2018-07-04T12:46:09Z

tools/sync_third_party.py

+
+root_path = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
+third_party_path = os.path.join(root_path, "third_party")
+script_name = "sync_third_party.py"


Rename file back to build_third_party.py
Use __file__ here

ry · 2018-07-04T12:47:58Z

tools/sync_third_party.py

+    print "Done (" + script_name + ")"
+
+def run(args):
+    print " ".join(args)


Run yapf across this file. I think there's inconstant quotation marks.

ry

LGTM after renaming to build_third_party.py again.

ry · 2018-07-04T12:49:21Z

README.md


-    (cd js; yarn install)
+Generate ninja configuration files.


s/configuration//

ry · 2018-07-04T12:50:38Z

tools/sync_third_party.py

+# - You need yarn installed as well.
+#   https://yarnpkg.com/lang/en/docs/install/
+# Use gclient_config.py to modify the git deps.
+# Use js/package.json to modify the npm deps.


Consider using absolute paths when talking about the configuration files:

Use //gclient_config.py to modify the git deps. Use //js/package.json to modify the npm deps.

…noland#328) Needed to solve denoland#20528 where we need to skip pumping V8 message loop for some time. Also adds "JsRuntime::with_event_loop" helper to run a future concurrently with the event loop.

kt3k added 2 commits July 4, 2018 14:05

chore: add tools/build_third_party.py

1902a8c

closes denoland#312

fix: update yarn cmd in .travis.yml

c44c431

mreinstein reviewed Jul 4, 2018

View reviewed changes

qti3e reviewed Jul 4, 2018

View reviewed changes

chore: update .gitignore

f3767ae

ry mentioned this pull request Jul 4, 2018

Add tools/sync.sh #327

Closed

ry reviewed Jul 4, 2018

View reviewed changes

kt3k added 3 commits July 4, 2018 21:20

chore: revert yarn.lock updates

c754ae9

refactor: update sync_third_party.py

65e0487

chore: update .travis.yml (use sync_third_party.py)

7496a63

ry reviewed Jul 4, 2018

View reviewed changes

ry approved these changes Jul 4, 2018

View reviewed changes

ry reviewed Jul 4, 2018

View reviewed changes

README.md Outdated

(cd js; yarn install)

Generate ninja configuration files.

Copy link

Member

ry Jul 4, 2018

Choose a reason for hiding this comment

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

s/configuration//

ry reviewed Jul 4, 2018

View reviewed changes

kt3k added 2 commits July 4, 2018 21:51

chore: change the script name back to build_third_party.py

acab43a

chore: better documentation

f6773a2

ry merged commit 2060bc9 into denoland:master Jul 4, 2018

kt3k deleted the feature/build-third-party branch July 4, 2018 13:11

kt3k mentioned this pull request Jul 8, 2018

Generate //third_party/ and the symlinks from the script. #346

Merged

piscisaureus pushed a commit to piscisaureus/deno that referenced this pull request Oct 7, 2019

Fix denoland#2064 (denoland#328)

2c11962

kt3k mentioned this pull request Oct 23, 2021

chore(node/_tools/setup): Improve cli messages denoland/std#1446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tools/build_third_party.py #328

Add tools/build_third_party.py #328

kt3k commented Jul 4, 2018

mreinstein Jul 4, 2018

mreinstein Jul 4, 2018

kt3k Jul 4, 2018

ry Jul 4, 2018

qti3e Jul 4, 2018

kt3k Jul 4, 2018

ry left a comment

ry Jul 4, 2018

kt3k Jul 4, 2018

ry Jul 4, 2018

ry Jul 4, 2018

ry Jul 4, 2018

ry Jul 4, 2018 •

edited

Loading

kt3k Jul 4, 2018

ry Jul 4, 2018

kt3k Jul 4, 2018

ry Jul 4, 2018

ry Jul 4, 2018 •

edited

Loading

ry Jul 4, 2018

ghost commented Jul 4, 2018 •

edited by ghost

Loading

ry commented Jul 4, 2018

ry commented Jul 4, 2018 •

edited

Loading

ry Jul 4, 2018 •

edited

Loading

ry Jul 4, 2018

ry left a comment

ry Jul 4, 2018

ry Jul 4, 2018

		@@ -54,12 +54,12 @@
		resolved "https://registry.yarnpkg.com/@types/long/-/long-3.0.32.tgz#f4e5af31e9e9b196d8e5fca8a5e2e20aa3d60b69"

Add tools/build_third_party.py #328

Add tools/build_third_party.py #328

Conversation

kt3k commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 4, 2018 • edited by ghost Loading

ry commented Jul 4, 2018

ry commented Jul 4, 2018 • edited Loading

ry Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry Jul 4, 2018 •

edited

Loading

ry Jul 4, 2018 •

edited

Loading

ghost commented Jul 4, 2018 •

edited by ghost

Loading

ry commented Jul 4, 2018 •

edited

Loading

ry Jul 4, 2018 •

edited

Loading