Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
556813003:
Add Go build support for GN. (Closed)
|
Created:
6 years, 3 months ago by tburkard Modified:
6 years, 2 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd Go build support for GN.
Provide a skeleton of the mojo system API, as well as a skeleton test.
R=qsr@chromium.org, brettw@chromium.org, viettrungluu@chromium.org
Committed: https://crrev.com/88a08a6c4dc21ac135f97707c062e97f36872b23
Cr-Commit-Position: refs/heads/master@{#297038}
Patch Set 1 #
Total comments: 30
Patch Set 2 : brettw feedback #Patch Set 3 : fix go group #
Total comments: 2
Patch Set 4 : qsr feedback #Patch Set 5 : got full gn build working #
Total comments: 10
Patch Set 6 : simply path computation #
Total comments: 1
Patch Set 7 : qsr feedback #
Total comments: 6
Patch Set 8 : qsr feedback #
Total comments: 5
Patch Set 9 : vtl feedback #
Total comments: 4
Patch Set 10 : qsr feedback #
Total comments: 6
Patch Set 11 : brettw feedback #
Total comments: 2
Messages
Total messages: 56 (6 generated)
Hi Brett, Hi Ben, I am sending you a first draft of adding Go support in Chromium using GN, as well as providing a skeleton of the mojo systems API in Go. Some things are clearly not right yet, eg the dependence of the GN rule on the output of the GYP (which is just hard coded for now, because I did not know how to do this). However, this compiles and works fine. Some things are really hacky, eg the way that Go requires absolute paths, which I had to generate in Python, b/c it is not supported in GN. In addition, Go does not permit to specify an output file for test binaries, and it is difficult to figure out what the output file name will be, which is why I just created the output in a new directory and copied the first file I found afterwards into the target). I'm looking forward to your comments, especially your ideas re: how to do some of the things described more elegantly. I will need more reviewers later on, but I wanted to share this with you first and incorporate your feedback first, before sending this to more people. Thanks.
On 2014/09/12 14:42:46, tburkard wrote: > Hi Brett, > Hi Ben, > > I am sending you a first draft of adding Go support in Chromium using GN, as > well as providing a skeleton of the mojo systems API in Go. > > Some things are clearly not right yet, eg the dependence of the GN rule on the > output of the GYP (which is just hard coded for now, because I did not know how > to do this). What do you mean by this, the -l target in rules.gni? > > However, this compiles and works fine. Some things are really hacky, eg the way > that Go requires absolute paths, which I had to generate in Python, b/c it is > not supported in GN. In addition, Go does not permit to specify an output file > for test binaries, and it is difficult to figure out what the output file name > will be, which is why I just created the output in a new directory and copied > the first file I found afterwards into the target). What about trying to use 6g and 6l yourself to build your targets? You would have control of all of your arguments, in particular the output files. Not sure how practical that is... > I'm looking forward to your comments, especially your ideas re: how to do some > of the things described more elegantly. > > I will need more reviewers later on, but I wanted to share this with you first > and incorporate your feedback first, before sending this to more people. > > Thanks.
https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py File build/go/go.py (right): https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode23 build/go/go.py:23: go_binary = args[1] You can use argpath. That would make it clearer what you are expected, and you will gain a --help for free. https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode30 build/go/go.py:30: go_path = os.path.abspath(src_root + "..") Instead of using concatenation, and so needing to / or not, use os.path.join https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode33 build/go/go.py:33: cgo_cflags = string.replace(" " + args[5], " -I", " -I" + src_root)[1:] If you expect this to be a list of -Ifoo -Ibar, just take foo and bar, and build the thing yourself. Same for -L https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode42 build/go/go.py:42: os.chdir(build_dir) Do you really need to chdir, doesn't the go compiler allows you to specify an output directory? https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/rules.gni File build/go/rules.gni (right): https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/rules.gni#ne... build/go/rules.gni:19: script = "//build/go/go.py" If you really cannot declare the target binary, then you should work in a temporary directory, and then find you executable and move it in the final location in the build_dir. You should have all of this generated by this template. https://chromiumcodereview.appspot.com/556813003/diff/1/mojo/mojo_base.gyp File mojo/mojo_base.gyp (right): https://chromiumcodereview.appspot.com/556813003/diff/1/mojo/mojo_base.gyp#ne... mojo/mojo_base.gyp:123: 'go/c_embedder/c_embedder.h', I don't think this is the right place for this. You should declare a library in the go/ directory that depends on this target and adds this.
Thanks for your feedback. Please find my comments below. Thanks. >> Some things are clearly not right yet, eg the dependence of the GN rule on the >> output of the GYP (which is just hard coded for now, because I did not know how >> to do this). > What do you mean by this, the -l target in rules.gni? Well, the lib that the rule depends on, is built by GYP. So how is this typically addressed, if you have some library built by GYP, but then it is referenced in a GNI? > What about trying to use 6g and 6l yourself to build your > targets? You would > have control of all of your arguments, in particular the > output files. Not sure > how practical that is... Yeah, at this point, I just wanted to get something working that's reliable and easy to customize. Looking at the go verbose output for invoking compiler & linker separately, it's quite elaborate, and probably subject to change, so I thought it was easier to just call the go build tool, and have it do everything. In particular, because I managed to get everything working with this change so that we can actually do it with GNI and get Go to produce the binaries we need. https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py File build/go/go.py (right): https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode23 build/go/go.py:23: go_binary = args[1] On 2014/09/15 09:54:24, qsr wrote: > You can use argpath. That would make it clearer what you are expected, and you > will gain a --help for free. You mean argparse? I was trying to do that, however, the python script then has to have knowledge of all the possible go args that may be passed in. I thought that might not be desirable, and to just specify a set of fixed args needed by the python script, and relay everything else to the go binary? But I am open to other suggestions. What do you think? https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode33 build/go/go.py:33: cgo_cflags = string.replace(" " + args[5], " -I", " -I" + src_root)[1:] On 2014/09/15 09:54:24, qsr wrote: > If you expect this to be a list of -Ifoo -Ibar, just take foo and bar, and build > the thing yourself. Same for -L Well, one issue is that Go works more consistently with absolute paths. So I'd still need to fix up the paths. But you are suggesting to provide this as a list of items relative to the src, and then use that to come up with the right -I? Again, one motivation why I started this way is so that the script does not have to know about potential other args that may be needed, but they will just be passed through. Thoughts? I am open to changing it based on your suggestion. So you are suggesting (in conjunction with the change above), something like --cgo_cflag_i=foo --cgo_cflag_i=bar, and in the python script, to translate that into -I/path/to/src/foo -I/path/to/src/bar? https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode42 build/go/go.py:42: os.chdir(build_dir) On 2014/09/15 09:54:23, qsr wrote: > Do you really need to chdir, doesn't the go compiler allows you to specify an > output directory? Go does support it for binaries, but not for test binaries! I double checked it with Russ Cox, and specifying a target is not supported at this time. This was the only way I could get it to work.
https://codereview.chromium.org/556813003/diff/1/build/go/rules.gni File build/go/rules.gni (right): https://codereview.chromium.org/556813003/diff/1/build/go/rules.gni#newcode19 build/go/rules.gni:19: script = "//build/go/go.py" I don't understand what you're saying here. https://codereview.chromium.org/556813003/diff/1/build/go/rules.gni#newcode23 build/go/rules.gni:23: "${go_build_tool}", Can you comment about this rule what this go test thingy is doing? Why do you need a separate build dir for this target? Generally the target_out_dir should be sufficient, but depending on how thin gs work, maybe not. https://codereview.chromium.org/556813003/diff/1/mojo/BUILD.gn File mojo/BUILD.gn (right): https://codereview.chromium.org/556813003/diff/1/mojo/BUILD.gn#newcode19 mojo/BUILD.gn:19: "//mojo/python", Sort https://codereview.chromium.org/556813003/diff/1/mojo/go/BUILD.gn File mojo/go/BUILD.gn (right): https://codereview.chromium.org/556813003/diff/1/mojo/go/BUILD.gn#newcode7 mojo/go/BUILD.gn:7: group("go") { I'm not really sure what this is supposed to be. I think I might have a mojo_use_go flag in a .gni file that you can use from various mojo places. Then you would declare the dependencies conditionally on that so the root mojo file wouldn't even reference this if go id disabled, and you can also remove the condition in the test rule on having the binary be undefined (you should assert that it's defined if you want go to run).
brettw feedback
Patchset #2 (id:20001) has been deleted
brettw feedback
Thanks, Brett and Ben for your comments. I addressed all of them. Sometimes, I asked questions/clarifications. Please let me know. Thanks. https://codereview.chromium.org/556813003/diff/1/build/go/rules.gni File build/go/rules.gni (right): https://codereview.chromium.org/556813003/diff/1/build/go/rules.gni#newcode19 build/go/rules.gni:19: script = "//build/go/go.py" On 2014/09/15 09:54:24, qsr wrote: > If you really cannot declare the target binary, then you should work in a > temporary directory, and then find you executable and move it in the final > location in the build_dir. You should have all of this generated by this > template. That's what I'm doing, I am creating the _build direcotry, perform the build in there, and then move the executable that was generated back to the target out dir. The python script is locating the file and copying it. I don't think I can do this in gn because I do not know the name of the output file. That's why I am doing it in python. https://codereview.chromium.org/556813003/diff/1/build/go/rules.gni#newcode23 build/go/rules.gni:23: "${go_build_tool}", On 2014/09/16 22:58:54, brettw wrote: > Can you comment about this rule what this go test thingy is doing? Why do you > need a separate build dir for this target? Generally the target_out_dir should > be sufficient, but depending on how thin gs work, maybe not. Explained why I am doing it this way in the comment that I just added. https://codereview.chromium.org/556813003/diff/1/mojo/BUILD.gn File mojo/BUILD.gn (right): https://codereview.chromium.org/556813003/diff/1/mojo/BUILD.gn#newcode19 mojo/BUILD.gn:19: "//mojo/python", On 2014/09/16 22:58:55, brettw wrote: > Sort Done. https://codereview.chromium.org/556813003/diff/1/mojo/go/BUILD.gn File mojo/go/BUILD.gn (right): https://codereview.chromium.org/556813003/diff/1/mojo/go/BUILD.gn#newcode7 mojo/go/BUILD.gn:7: group("go") { On 2014/09/16 22:58:55, brettw wrote: > I'm not really sure what this is supposed to be. This group("go") was copied from qsr's python stuff. Ben, what is the purpose of group("python")? > I think I might have a mojo_use_go flag in a .gni file that you can use from > various mojo places. Then you would declare the dependencies conditionally on > that so the root mojo file wouldn't even reference this if go id disabled, and > you can also remove the condition in the test rule on having the binary be > undefined (you should assert that it's defined if you want go to run). Done.
https://codereview.chromium.org/556813003/diff/1/mojo/go/BUILD.gn File mojo/go/BUILD.gn (right): https://codereview.chromium.org/556813003/diff/1/mojo/go/BUILD.gn#newcode7 mojo/go/BUILD.gn:7: group("go") { On 2014/09/17 12:06:45, tburkard wrote: > On 2014/09/16 22:58:55, brettw wrote: > > I'm not really sure what this is supposed to be. > > This group("go") was copied from qsr's python stuff. Ben, what is the purpose of > group("python")? I do not think I have an empty group anywhere. Which one are you referring to? > > I think I might have a mojo_use_go flag in a .gni file that you can use from > > various mojo places. Then you would declare the dependencies conditionally on > > that so the root mojo file wouldn't even reference this if go id disabled, and > > you can also remove the condition in the test rule on having the binary be > > undefined (you should assert that it's defined if you want go to run). > > Done.
fix go group
Fixed the go group after talking with Ben, and understanding how groups work. Please let me know your thoughts on my still pending responses. Thanks. https://codereview.chromium.org/556813003/diff/1/mojo/go/BUILD.gn File mojo/go/BUILD.gn (right): https://codereview.chromium.org/556813003/diff/1/mojo/go/BUILD.gn#newcode7 mojo/go/BUILD.gn:7: group("go") { On 2014/09/17 12:30:25, qsr wrote: > On 2014/09/17 12:06:45, tburkard wrote: > > On 2014/09/16 22:58:55, brettw wrote: > > > I'm not really sure what this is supposed to be. > > > > This group("go") was copied from qsr's python stuff. Ben, what is the purpose > of > > group("python")? > > I do not think I have an empty group anywhere. Which one are you referring to? > > > > I think I might have a mojo_use_go flag in a .gni file that you can use from > > > various mojo places. Then you would declare the dependencies conditionally > on > > > that so the root mojo file wouldn't even reference this if go id disabled, > and > > > you can also remove the condition in the test rule on having the binary be > > > undefined (you should assert that it's defined if you want go to run). > > > > Done. > Fixed per discussion with qsr.
Brett, Ben: so my main question, besides the few open points that I mentioned above, is how to express (in GN) the dependency on the GYP target (the .so). Right now, the -I and -L are just hard coded, and there is no real dependency expressed (it just uses the GYP targets and assumes they are there). Is it possible for the GN file to depend on the GYP target? Is there some cross-language example of non-C++ stuff depending on a C++ .so? Thanks. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py File build/go/go.py (right): https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode23 build/go/go.py:23: go_binary = args[1] On 2014/09/16 12:29:13, tburkard wrote: > On 2014/09/15 09:54:24, qsr wrote: > > You can use argpath. That would make it clearer what you are expected, and you > > will gain a --help for free. > > You mean argparse? I was trying to do that, however, the python script then has > to have knowledge of all the possible go args that may be passed in. I thought > that might not be desirable, and to just specify a set of fixed args needed by > the python script, and relay everything else to the go binary? > > But I am open to other suggestions. What do you think? I'm not sure to understand, you can do something like this: parser.add_argument('a1') parser.add_argument('a2') parser.add_argument('a3') parser.add_argument('remaining', nargs='*') Which would name your 3 first argument on the command line, and let you get all the following ones in remaining. https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode33 build/go/go.py:33: cgo_cflags = string.replace(" " + args[5], " -I", " -I" + src_root)[1:] On 2014/09/16 12:29:13, tburkard wrote: > On 2014/09/15 09:54:24, qsr wrote: > > If you expect this to be a list of -Ifoo -Ibar, just take foo and bar, and > build > > the thing yourself. Same for -L > > Well, one issue is that Go works more consistently with absolute paths. So I'd > still need to fix up the paths. But you are suggesting to provide this as a list > of items relative to the src, and then use that to come up with the right -I? > Again, one motivation why I started this way is so that the script does not have > to know about potential other args that may be needed, but they will just be > passed through. > > Thoughts? I am open to changing it based on your suggestion. > So you are suggesting (in conjunction with the change above), something like > --cgo_cflag_i=foo --cgo_cflag_i=bar, and in the python script, to translate that > into -I/path/to/src/foo -I/path/to/src/bar? Hum, ok I see. Then should this script hardcode the -I and -L it needs to add. You could pass the output_dir to the script, and hardcode -I{src_dir} -L{output_dir} Then you can just have 2 optional arguments go_clfags and go_dflags and construct the final cflags and dflags from those. https://chromiumcodereview.appspot.com/556813003/diff/1/mojo/go/BUILD.gn File mojo/go/BUILD.gn (right): https://chromiumcodereview.appspot.com/556813003/diff/1/mojo/go/BUILD.gn#newc... mojo/go/BUILD.gn:14: "tests/system_test.go", Why don't you depend on all the other .go files? Also, how would it work in the end, can you do go libraries and depend on that in final executable? Otherwise, will you need to repeat the dependencies on go files? Or can you use source_set to define you libraries? I do not know how source_set and templates interact. https://chromiumcodereview.appspot.com/556813003/diff/60001/mojo/go/BUILD.gn File mojo/go/BUILD.gn (right): https://chromiumcodereview.appspot.com/556813003/diff/60001/mojo/go/BUILD.gn#... mojo/go/BUILD.gn:22: # "//mojo/public/python:system", Why do you depend on anything python here?
https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py File build/go/go.py (right): https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode23 build/go/go.py:23: go_binary = args[1] On 2014/09/18 08:30:38, qsr wrote: > On 2014/09/16 12:29:13, tburkard wrote: > > On 2014/09/15 09:54:24, qsr wrote: > > > You can use argpath. That would make it clearer what you are expected, and > you > > > will gain a --help for free. > > > > You mean argparse? I was trying to do that, however, the python script then > has > > to have knowledge of all the possible go args that may be passed in. I thought > > that might not be desirable, and to just specify a set of fixed args needed by > > the python script, and relay everything else to the go binary? > > > > But I am open to other suggestions. What do you think? > > I'm not sure to understand, you can do something like this: > > parser.add_argument('a1') > parser.add_argument('a2') > parser.add_argument('a3') > parser.add_argument('remaining', nargs='*') > > Which would name your 3 first argument on the command line, and let you get all > the following ones in remaining. Yup, I tried doing that, however, if 'remaining' contains leading dash(es), there will be an error. So I could use argparse, but I would have to hard code in the semantics of all flags passed to go, which is what I wanted to avoid. Would you prefer doing that? https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode33 build/go/go.py:33: cgo_cflags = string.replace(" " + args[5], " -I", " -I" + src_root)[1:] On 2014/09/18 08:30:38, qsr wrote: > On 2014/09/16 12:29:13, tburkard wrote: > > On 2014/09/15 09:54:24, qsr wrote: > > > If you expect this to be a list of -Ifoo -Ibar, just take foo and bar, and > > build > > > the thing yourself. Same for -L > > > > Well, one issue is that Go works more consistently with absolute paths. So I'd > > still need to fix up the paths. But you are suggesting to provide this as a > list > > of items relative to the src, and then use that to come up with the right -I? > > Again, one motivation why I started this way is so that the script does not > have > > to know about potential other args that may be needed, but they will just be > > passed through. > > > > Thoughts? I am open to changing it based on your suggestion. > > So you are suggesting (in conjunction with the change above), something like > > --cgo_cflag_i=foo --cgo_cflag_i=bar, and in the python script, to translate > that > > into -I/path/to/src/foo -I/path/to/src/bar? > > Hum, ok I see. Then should this script hardcode the -I and -L it needs to add. > You could pass the output_dir to the script, and hardcode -I{src_dir} > -L{output_dir} > > Then you can just have 2 optional arguments go_clfags and go_dflags and > construct the final cflags and dflags from those. .. but there's a bit more to it than just the output_dir: First, lib's/so's could be in different subdirectories of an output directory. So it'd have to be a list. Second, I also need to pass in the -l libs. So in the end, it would look pretty similar to what I already have? Does this sound reasonable? https://chromiumcodereview.appspot.com/556813003/diff/1/mojo/go/BUILD.gn File mojo/go/BUILD.gn (right): https://chromiumcodereview.appspot.com/556813003/diff/1/mojo/go/BUILD.gn#newc... mojo/go/BUILD.gn:14: "tests/system_test.go", On 2014/09/18 08:30:38, qsr wrote: > Why don't you depend on all the other .go files? > > Also, how would it work in the end, can you do go libraries and depend on that > in final executable? Otherwise, will you need to repeat the dependencies on go > files? > > Or can you use source_set to define you libraries? I do not know how source_set > and templates interact. Yup, that's what I'm not sure, how to express these dependencies. Dependencies on (i) the other go code (ii) the GYP lib. Brett, can you provide some input perhaps? Maybe easier to set up some time to chat, what do you think? Thanks. https://chromiumcodereview.appspot.com/556813003/diff/60001/mojo/go/BUILD.gn File mojo/go/BUILD.gn (right): https://chromiumcodereview.appspot.com/556813003/diff/60001/mojo/go/BUILD.gn#... mojo/go/BUILD.gn:22: # "//mojo/public/python:system", On 2014/09/18 08:30:38, qsr wrote: > Why do you depend on anything python here? I had copied this from your python template. I wasn't sure how this works here in go. I know I will have to depend on the other go files. Are these deps or datadeps (datadeps perhaps because they are not targets that need to be compiled)? See comment to other question.
https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py File build/go/go.py (right): https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode23 build/go/go.py:23: go_binary = args[1] On 2014/09/18 14:23:42, tburkard wrote: > On 2014/09/18 08:30:38, qsr wrote: > > On 2014/09/16 12:29:13, tburkard wrote: > > > On 2014/09/15 09:54:24, qsr wrote: > > > > You can use argpath. That would make it clearer what you are expected, and > > you > > > > will gain a --help for free. > > > > > > You mean argparse? I was trying to do that, however, the python script then > > has > > > to have knowledge of all the possible go args that may be passed in. I > thought > > > that might not be desirable, and to just specify a set of fixed args needed > by > > > the python script, and relay everything else to the go binary? > > > > > > But I am open to other suggestions. What do you think? > > > > I'm not sure to understand, you can do something like this: > > > > parser.add_argument('a1') > > parser.add_argument('a2') > > parser.add_argument('a3') > > parser.add_argument('remaining', nargs='*') > > > > Which would name your 3 first argument on the command line, and let you get > all > > the following ones in remaining. > > Yup, I tried doing that, however, if 'remaining' contains leading dash(es), > there will be an error. So I could use argparse, but I would have to hard code > in the semantics of all flags passed to go, which is what I wanted to avoid. > Would you prefer doing that? You can use -- to tell argparse all arguments is positional. So you will use you go.py like this: go.py --my_arg1=1 --my_arg2=1 -- --my-arg3 --my-arg4 https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode33 build/go/go.py:33: cgo_cflags = string.replace(" " + args[5], " -I", " -I" + src_root)[1:] On 2014/09/18 14:23:41, tburkard wrote: > On 2014/09/18 08:30:38, qsr wrote: > > On 2014/09/16 12:29:13, tburkard wrote: > > > On 2014/09/15 09:54:24, qsr wrote: > > > > If you expect this to be a list of -Ifoo -Ibar, just take foo and bar, and > > > build > > > > the thing yourself. Same for -L > > > > > > Well, one issue is that Go works more consistently with absolute paths. So > I'd > > > still need to fix up the paths. But you are suggesting to provide this as a > > list > > > of items relative to the src, and then use that to come up with the right > -I? > > > Again, one motivation why I started this way is so that the script does not > > have > > > to know about potential other args that may be needed, but they will just be > > > passed through. > > > > > > Thoughts? I am open to changing it based on your suggestion. > > > So you are suggesting (in conjunction with the change above), something like > > > --cgo_cflag_i=foo --cgo_cflag_i=bar, and in the python script, to translate > > that > > > into -I/path/to/src/foo -I/path/to/src/bar? > > > > Hum, ok I see. Then should this script hardcode the -I and -L it needs to > add. > > You could pass the output_dir to the script, and hardcode -I{src_dir} > > -L{output_dir} > > > > Then you can just have 2 optional arguments go_clfags and go_dflags and > > construct the final cflags and dflags from those. > > .. but there's a bit more to it than just the output_dir: First, lib's/so's > could be in different subdirectories of an output directory. So it'd have to be > a list. That I do not understand, your libs should all be in OUTPUT_DIR/lib on linux. Where else do you expect to get libraries? > Second, I also need to pass in the -l libs. Those would be there yes, as the remaining arguments. But those do not contain path, so you should just pass back what has been given to you, and you will not need to change this to add absolute paths.
https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py File build/go/go.py (right): https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode23 build/go/go.py:23: go_binary = args[1] On 2014/09/18 14:30:55, qsr wrote: > On 2014/09/18 14:23:42, tburkard wrote: > > On 2014/09/18 08:30:38, qsr wrote: > > > On 2014/09/16 12:29:13, tburkard wrote: > > > > On 2014/09/15 09:54:24, qsr wrote: > > > > > You can use argpath. That would make it clearer what you are expected, > and > > > you > > > > > will gain a --help for free. > > > > > > > > You mean argparse? I was trying to do that, however, the python script > then > > > has > > > > to have knowledge of all the possible go args that may be passed in. I > > thought > > > > that might not be desirable, and to just specify a set of fixed args > needed > > by > > > > the python script, and relay everything else to the go binary? > > > > > > > > But I am open to other suggestions. What do you think? > > > > > > I'm not sure to understand, you can do something like this: > > > > > > parser.add_argument('a1') > > > parser.add_argument('a2') > > > parser.add_argument('a3') > > > parser.add_argument('remaining', nargs='*') > > > > > > Which would name your 3 first argument on the command line, and let you get > > all > > > the following ones in remaining. > > > > Yup, I tried doing that, however, if 'remaining' contains leading dash(es), > > there will be an error. So I could use argparse, but I would have to hard code > > in the semantics of all flags passed to go, which is what I wanted to avoid. > > Would you prefer doing that? > > You can use -- to tell argparse all arguments is positional. So you will use > you go.py like this: > go.py --my_arg1=1 --my_arg2=1 -- --my-arg3 --my-arg4 Done. https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode30 build/go/go.py:30: go_path = os.path.abspath(src_root + "..") On 2014/09/15 09:54:24, qsr wrote: > Instead of using concatenation, and so needing to / or not, use os.path.join Done. https://chromiumcodereview.appspot.com/556813003/diff/1/build/go/go.py#newcode33 build/go/go.py:33: cgo_cflags = string.replace(" " + args[5], " -I", " -I" + src_root)[1:] On 2014/09/18 14:30:55, qsr wrote: > On 2014/09/18 14:23:41, tburkard wrote: > > On 2014/09/18 08:30:38, qsr wrote: > > > On 2014/09/16 12:29:13, tburkard wrote: > > > > On 2014/09/15 09:54:24, qsr wrote: > > > > > If you expect this to be a list of -Ifoo -Ibar, just take foo and bar, > and > > > > build > > > > > the thing yourself. Same for -L > > > > > > > > Well, one issue is that Go works more consistently with absolute paths. So > > I'd > > > > still need to fix up the paths. But you are suggesting to provide this as > a > > > list > > > > of items relative to the src, and then use that to come up with the right > > -I? > > > > Again, one motivation why I started this way is so that the script does > not > > > have > > > > to know about potential other args that may be needed, but they will just > be > > > > passed through. > > > > > > > > Thoughts? I am open to changing it based on your suggestion. > > > > So you are suggesting (in conjunction with the change above), something > like > > > > --cgo_cflag_i=foo --cgo_cflag_i=bar, and in the python script, to > translate > > > that > > > > into -I/path/to/src/foo -I/path/to/src/bar? > > > > > > Hum, ok I see. Then should this script hardcode the -I and -L it needs to > > add. > > > You could pass the output_dir to the script, and hardcode -I{src_dir} > > > -L{output_dir} > > > > > > Then you can just have 2 optional arguments go_clfags and go_dflags and > > > construct the final cflags and dflags from those. > > > > .. but there's a bit more to it than just the output_dir: First, lib's/so's > > could be in different subdirectories of an output directory. So it'd have to > be > > a list. > > That I do not understand, your libs should all be in OUTPUT_DIR/lib on linux. > Where else do you expect to get libraries? > > > Second, I also need to pass in the -l libs. > > Those would be there yes, as the remaining arguments. But those do not contain > path, so you should just pass back what has been given to you, and you will not > need to change this to add absolute paths. Ah, yes, that makes sense. So I'd have a positional argument indicating the (single) path. And I'd fix that up. The -l ones will be pased in the remaining options. Does this sound reasonable? In the general case though, couldn't libs come from multiple places? Eg what if there is the gyp out, and the gn out, wouldn't libs be in separate direcotires? Maybe Brett can chime in on this before i make the change. Thanks.
> In the general case though, couldn't libs come from multiple places? Eg what if > there is the gyp out, and the gn out, wouldn't libs be in separate direcotires? You cannot have gn build and gyp build mixing artifact from one another. That would not work, as you do not expect people to use both at the same time. You should change your dependency to depends on library build by gn. mojo_system_impl is //mojo/system lbase is //base
On 2014/09/18 15:24:49, qsr wrote: > > In the general case though, couldn't libs come from multiple places? Eg what > if > > there is the gyp out, and the gn out, wouldn't libs be in separate > direcotires? > > You cannot have gn build and gyp build mixing artifact from one another. That > would not work, as you do not expect people to use both at the same time. You > should change your dependency to depends on library build by gn. > mojo_system_impl is //mojo/system > lbase is //base There is no static library or .so created by the current BUILD.gn though, unlike in the gyp target. I guess I'll just add a static library then for my use case? Or is there a preferred way to do this (or am I missing something?) Thanks.
got full gn build working
got full gn build working
Patchset #5 (id:100001) has been deleted
Hi, I removed the depence on gyp, and all the dependencies are now expressed correctly, and it will build from a clean out directory. The one thing I'm not sure yet: -I, -L, and -l are hard coded in rules.gni for now. Can these be generated automatically somehow based on the dependence? Or should they go into BUILD.gn instead rather? Please advise, and please let me know if there are any other concerns. Thanks.
fix path
Ok, this fixed the hard coded path. Please let me know if you have any other comments. Thanks.
simply path computation
https://chromiumcodereview.appspot.com/556813003/diff/120001/build/go/rules.gni File build/go/rules.gni (right): https://chromiumcodereview.appspot.com/556813003/diff/120001/build/go/rules.g... build/go/rules.gni:14: # assert(defined(invoker.go_base_module)) You should remove this if this is not needed. https://chromiumcodereview.appspot.com/556813003/diff/120001/build/go/rules.g... build/go/rules.gni:42: "-I" + rebase_path("//", "//"), Use rebase_path without second argument to directly get absolute path, that will simplify this and go.py a lot. https://chromiumcodereview.appspot.com/556813003/diff/120001/mojo/go/system/i... File mojo/go/system/impl/core_impl.go (right): https://chromiumcodereview.appspot.com/556813003/diff/120001/mojo/go/system/i... mojo/go/system/impl/core_impl.go:13: func (c CoreImpl) GetTimeTicksNow() int64 { Do you have typedef or equivalent in go? I understand that is just a proof of concept, but when you will work on the real library, you might want to have typedef for this return type. https://chromiumcodereview.appspot.com/556813003/diff/120001/mojo/go/system/i... mojo/go/system/impl/core_impl.go:20: if lazyInstance == nil { Do you need any kind of thread safety here? https://chromiumcodereview.appspot.com/556813003/diff/120001/mojo/go/tests/sy... File mojo/go/tests/system_test.go (right): https://chromiumcodereview.appspot.com/556813003/diff/120001/mojo/go/tests/sy... mojo/go/tests/system_test.go:17: t.Error("hi"); You might want a better error.
Patchset #6 (id:140001) has been deleted
https://chromiumcodereview.appspot.com/556813003/diff/160001/build/go/go.py File build/go/go.py (right): https://chromiumcodereview.appspot.com/556813003/diff/160001/build/go/go.py#n... build/go/go.py:37: src_root = os.path.abspath(args.src_root) + "/" You do not need this anymore. Do: go_path = os.path.abspath(os.path.join(args.src_root, ".."))
qsr feedback
https://chromiumcodereview.appspot.com/556813003/diff/120001/build/go/rules.gni File build/go/rules.gni (right): https://chromiumcodereview.appspot.com/556813003/diff/120001/build/go/rules.g... build/go/rules.gni:14: # assert(defined(invoker.go_base_module)) On 2014/09/22 11:11:25, qsr wrote: > You should remove this if this is not needed. Done. https://chromiumcodereview.appspot.com/556813003/diff/120001/build/go/rules.g... build/go/rules.gni:42: "-I" + rebase_path("//", "//"), On 2014/09/22 11:11:25, qsr wrote: > Use rebase_path without second argument to directly get absolute path, that will > simplify this and go.py a lot. Done. https://chromiumcodereview.appspot.com/556813003/diff/120001/mojo/go/system/i... File mojo/go/system/impl/core_impl.go (right): https://chromiumcodereview.appspot.com/556813003/diff/120001/mojo/go/system/i... mojo/go/system/impl/core_impl.go:13: func (c CoreImpl) GetTimeTicksNow() int64 { On 2014/09/22 11:11:25, qsr wrote: > Do you have typedef or equivalent in go? I understand that is just a proof of > concept, but when you will work on the real library, you might want to have > typedef for this return type. I will address this in a follow-on change, which I will have reviewed by someone who is very experienced in go. https://chromiumcodereview.appspot.com/556813003/diff/120001/mojo/go/system/i... mojo/go/system/impl/core_impl.go:20: if lazyInstance == nil { On 2014/09/22 11:11:25, qsr wrote: > Do you need any kind of thread safety here? I will address this in a follow-on change, which I will have reviewed by someone who is very experienced in go. https://chromiumcodereview.appspot.com/556813003/diff/120001/mojo/go/tests/sy... File mojo/go/tests/system_test.go (right): https://chromiumcodereview.appspot.com/556813003/diff/120001/mojo/go/tests/sy... mojo/go/tests/system_test.go:17: t.Error("hi"); On 2014/09/22 11:11:25, qsr wrote: > You might want a better error. Done.
https://chromiumcodereview.appspot.com/556813003/diff/180001/build/go/go.py File build/go/go.py (right): https://chromiumcodereview.appspot.com/556813003/diff/180001/build/go/go.py#n... build/go/go.py:19: import string This import is not used. https://chromiumcodereview.appspot.com/556813003/diff/180001/build/go/go.py#n... build/go/go.py:37: src_root = os.path.abspath(args.src_root) + "/" You do not need this concantenation or this local variable. https://chromiumcodereview.appspot.com/556813003/diff/180001/mojo/public/plat... File mojo/public/platform/native/system_thunks.h (right): https://chromiumcodereview.appspot.com/556813003/diff/180001/mojo/public/plat... mojo/public/platform/native/system_thunks.h:47: const struct MojoCreateMessagePipeOptions* options, Please add Trung as a reviewer for those changes.
qsr feedback
tburkard@chromium.org changed reviewers: + viettrungluu@chromium.org
Thanks, Ben. Incorporated all your comments. Viet-Trung, can you please take a look? Primarily at the system_thunks.h changes, but probably also at everything else if you are interested. Thanks. https://chromiumcodereview.appspot.com/556813003/diff/180001/build/go/go.py File build/go/go.py (right): https://chromiumcodereview.appspot.com/556813003/diff/180001/build/go/go.py#n... build/go/go.py:19: import string On 2014/09/22 14:35:07, qsr wrote: > This import is not used. Done. https://chromiumcodereview.appspot.com/556813003/diff/180001/build/go/go.py#n... build/go/go.py:37: src_root = os.path.abspath(args.src_root) + "/" On 2014/09/22 14:35:07, qsr wrote: > You do not need this concantenation or this local variable. Done. https://chromiumcodereview.appspot.com/556813003/diff/180001/mojo/public/plat... File mojo/public/platform/native/system_thunks.h (right): https://chromiumcodereview.appspot.com/556813003/diff/180001/mojo/public/plat... mojo/public/platform/native/system_thunks.h:47: const struct MojoCreateMessagePipeOptions* options, On 2014/09/22 14:35:07, qsr wrote: > Please add Trung as a reviewer for those changes. Done.
lgtm
https://codereview.chromium.org/556813003/diff/200001/mojo/public/platform/na... File mojo/public/platform/native/system_thunks.h (right): https://codereview.chromium.org/556813003/diff/200001/mojo/public/platform/na... mojo/public/platform/native/system_thunks.h:4: Probably you should add a comment indicating that this header should be valid C (just like the headers in mojo/public/c/system have such a comment). https://codereview.chromium.org/556813003/diff/200001/mojo/public/platform/na... mojo/public/platform/native/system_thunks.h:104: inline struct MojoSystemThunks MojoMakeSystemThunks() { ... though "inline" requires either C99 (or a compiler extension), if it's C. Also, if it's to be C, you need an explicit "void" parameter. The interpretation of |inline| also differs between C99 and C++. This is troublesome since C99 seems to require that you say "extern inline struct MojoSystemThunks MojoMakeSystemThunks(void)" in exactly one .c file (compilation unit). Since this is only called from the embedder, you shouldn't need it and the best thing to do is to simply put it inside |#ifndef __cplusplus ... #endif|, and not change it at all.
https://chromiumcodereview.appspot.com/556813003/diff/200001/mojo/public/plat... File mojo/public/platform/native/system_thunks.h (right): https://chromiumcodereview.appspot.com/556813003/diff/200001/mojo/public/plat... mojo/public/platform/native/system_thunks.h:4: On 2014/09/23 13:44:43, viettrungluu wrote: > Probably you should add a comment indicating that this header should be valid C > (just like the headers in mojo/public/c/system have such a comment). Done. https://chromiumcodereview.appspot.com/556813003/diff/200001/mojo/public/plat... mojo/public/platform/native/system_thunks.h:104: inline struct MojoSystemThunks MojoMakeSystemThunks() { On 2014/09/23 13:44:43, viettrungluu wrote: > ... though "inline" requires either C99 (or a compiler extension), if it's C. > > Also, if it's to be C, you need an explicit "void" parameter. > > The interpretation of |inline| also differs between C99 and C++. This is > troublesome since C99 seems to require that you say "extern inline struct > MojoSystemThunks MojoMakeSystemThunks(void)" in exactly one .c file (compilation > unit). > > Since this is only called from the embedder, you shouldn't need it and the best > thing to do is to simply put it inside |#ifndef __cplusplus ... #endif|, and not > change it at all. Done.
vtl feedback
https://chromiumcodereview.appspot.com/556813003/diff/200001/mojo/public/plat... File mojo/public/platform/native/system_thunks.h (right): https://chromiumcodereview.appspot.com/556813003/diff/200001/mojo/public/plat... mojo/public/platform/native/system_thunks.h:104: inline struct MojoSystemThunks MojoMakeSystemThunks() { On 2014/09/23 13:52:21, tburkard wrote: > On 2014/09/23 13:44:43, viettrungluu wrote: > > ... though "inline" requires either C99 (or a compiler extension), if it's C. > > > > Also, if it's to be C, you need an explicit "void" parameter. > > > > The interpretation of |inline| also differs between C99 and C++. This is > > troublesome since C99 seems to require that you say "extern inline struct > > MojoSystemThunks MojoMakeSystemThunks(void)" in exactly one .c file > (compilation > > unit). > > > > Since this is only called from the embedder, you shouldn't need it and the > best > > thing to do is to simply put it inside |#ifndef __cplusplus ... #endif|, and > not > > change it at all. > > Done. Should it really be ifndef? or ifdef? Thanks.
https://codereview.chromium.org/556813003/diff/220001/mojo/public/platform/na... File mojo/public/platform/native/system_thunks.h (right): https://codereview.chromium.org/556813003/diff/220001/mojo/public/platform/na... mojo/public/platform/native/system_thunks.h:5: // Note: This header should be compilable as C. You added this comment twice. https://codereview.chromium.org/556813003/diff/220001/mojo/public/platform/na... mojo/public/platform/native/system_thunks.h:107: #ifndef __cplusplus That should be #ifdef, and you should not modify the content inside this.
qsr feedback
Patchset #10 (id:240001) has been deleted
qsr feedback
https://chromiumcodereview.appspot.com/556813003/diff/220001/mojo/public/plat... File mojo/public/platform/native/system_thunks.h (right): https://chromiumcodereview.appspot.com/556813003/diff/220001/mojo/public/plat... mojo/public/platform/native/system_thunks.h:5: // Note: This header should be compilable as C. On 2014/09/23 14:20:49, qsr wrote: > You added this comment twice. Done. Whoops, one of the other files, functions.h, contained it twice, too, but others don't. fixed. https://chromiumcodereview.appspot.com/556813003/diff/220001/mojo/public/plat... mojo/public/platform/native/system_thunks.h:107: #ifndef __cplusplus On 2014/09/23 14:20:49, qsr wrote: > That should be #ifdef, and you should not modify the content inside this. Done.
Ping. Brett/Viet-Trung, any more comments? Thanks.
lgtm
https://codereview.chromium.org/556813003/diff/260001/build/go/rules.gni File build/go/rules.gni (right): https://codereview.chromium.org/556813003/diff/260001/build/go/rules.gni#newc... build/go/rules.gni:10: template("go_test_binary") { This needs documentation. What variables does it expect and what do they mean. Are they optional? https://codereview.chromium.org/556813003/diff/260001/build/go/rules.gni#newc... build/go/rules.gni:19: sources = invoker.additional_sources This requires "additional_sources" be set. Will there always be additional sources? That isn't implied to me in the name. I believe static libs will get confused if there are no sources. Do we need to handle these? https://codereview.chromium.org/556813003/diff/260001/mojo/go/system/embedder... File mojo/go/system/embedder/embedder.go (right): https://codereview.chromium.org/556813003/diff/260001/mojo/go/system/embedder... mojo/go/system/embedder/embedder.go:1: package embedder; Seems like these new files should have copyrights.
brettw feedback
This should address all the points you mentioned, Brett. Please take another look. Thanks. https://chromiumcodereview.appspot.com/556813003/diff/260001/build/go/rules.gni File build/go/rules.gni (right): https://chromiumcodereview.appspot.com/556813003/diff/260001/build/go/rules.g... build/go/rules.gni:10: template("go_test_binary") { On 2014/09/24 19:25:45, brettw wrote: > This needs documentation. What variables does it expect and what do they mean. > Are they optional? Done. https://chromiumcodereview.appspot.com/556813003/diff/260001/build/go/rules.g... build/go/rules.gni:19: sources = invoker.additional_sources On 2014/09/24 19:25:45, brettw wrote: > This requires "additional_sources" be set. Will there always be additional > sources? That isn't implied to me in the name. I believe static libs will get > confused if there are no sources. Do we need to handle these? In general, I think any mojo code will require interfacing with C code, so I think it's ok to require this. However, I agree with you on the name, so I renamed it to something more descriptive. Optionally, we could permit having no static_library_sources, but do you know how this could be done? If not, we can punt on this for now and require it. Thanks. https://chromiumcodereview.appspot.com/556813003/diff/260001/mojo/go/system/e... File mojo/go/system/embedder/embedder.go (right): https://chromiumcodereview.appspot.com/556813003/diff/260001/mojo/go/system/e... mojo/go/system/embedder/embedder.go:1: package embedder; On 2014/09/24 19:25:45, brettw wrote: > Seems like these new files should have copyrights. Done.
https://chromiumcodereview.appspot.com/556813003/diff/280001/build/go/rules.gni File build/go/rules.gni (right): https://chromiumcodereview.appspot.com/556813003/diff/280001/build/go/rules.g... build/go/rules.gni:32: sources = invoker.static_library_sources I do not think you should force C sources. When the core API will be done, the rest should be pure go. You should check if the variable exists. You need to check defined(invoker.static_library_sources)
LGTM https://chromiumcodereview.appspot.com/556813003/diff/280001/build/go/rules.gni File build/go/rules.gni (right): https://chromiumcodereview.appspot.com/556813003/diff/280001/build/go/rules.g... build/go/rules.gni:32: sources = invoker.static_library_sources On 2014/09/26 13:33:41, qsr wrote: > I do not think you should force C sources. When the core API will be done, the > rest should be pure go. You should check if the variable exists. You need to > check defined(invoker.static_library_sources) The problem is you can't make a static library if there are no sources. If you want to support "no C sources" you either need to use a source set (which would then not be linkable with your script), or have a mode where it doesn't produce a static library and you presumably don't lass the "-l" to the script.
The CQ bit was checked by tburkard@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/556813003/280001
Message was sent while issue was closed.
Committed patchset #11 (id:280001) as e9985c230799b689b97359f6243300acaebe66b1
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/88a08a6c4dc21ac135f97707c062e97f36872b23 Cr-Commit-Position: refs/heads/master@{#297038} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
