Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1892243002:
Write a phony target for the top-level directory of a create_bundle target. (Closed)
|
Created:
4 years, 8 months ago by Robert Sesek Modified:
4 years, 8 months ago CC:
chromium-reviews, tfarina, sdefresne Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWrite a phony target for the top-level directory of a create_bundle target.
This is needed so that if a create_bundle is used as a bundle_data in another
bundle, the dependent bundle can be specified as a sources.
BUG=297668
R=brettw@chromium.org
Committed: https://crrev.com/a23feb50e97c016318d5a859609f8f6a139f0ed5
Cr-Commit-Position: refs/heads/master@{#389101}
Patch Set 1 #
Total comments: 12
Patch Set 2 : build_dir #Patch Set 3 : Address comments #
Dependent Patchsets: Messages
Total messages: 20 (6 generated)
Description was changed from ========== Write a phony target for the top-level directory of a create_bundle target. This is needed so that if a create_bundle is used as a bundle_data in another bundle, the dependent bundle can be specified as a sources. BUG=297668 R=brettw@chromium.org ========== to ========== Write a phony target for the top-level directory of a create_bundle target. This is needed so that if a create_bundle is used as a bundle_data in another bundle, the dependent bundle can be specified as a sources. BUG=297668 R=brettw@chromium.org ==========
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
I'm a bit confused by this change. I can see that you're creating a 'bar.bundle' rule, but what do you mean by "the dependent bundle can be specified as a sources"? I feel like either I don't understand what you're trying to do, or we've got the dependencies wrong somehow, because creating a phony target in ninja shouldn't affect the list of sources or how we specify dependencies.
On 2016/04/15 22:05:01, Dirk Pranke wrote:
> I'm a bit confused by this change. I can see that you're creating a
'bar.bundle'
> rule, but what do you mean by "the dependent bundle can be specified as a
> sources"?
>
> I feel like either I don't understand what you're trying to do, or we've got
the
> dependencies wrong somehow, because creating a phony target in ninja shouldn't
> affect the list of sources or how we specify dependencies.
This is needed for the way Content Shell and Chromium work. Each have a
.framework bundle that then needs to be a bundle_data of the .app bundle.
Currently the outputs of a bundle only list the individual files that go into
the bundle, and it doesn't treat the fully packaged bundle as a discrete output.
It is necessary to treat the output of create_bundle as a single unit so that it
can be copied into another bundle.
E.g., before this change:
outputs:
//out/gn2/Content Shell.app/Contents/Frameworks/Content Shell
Framework.framework
//out/gn2/Content Shell.app/Contents/Frameworks/Content Shell Helper.app
//out/gn2/Content Shell.app/Contents/Info.plist
//out/gn2/Content Shell.app/Contents/MacOS/Content Shell
//out/gn2/Content Shell.app/Contents/Resources/app.icns
With this change:
outputs:
//out/gn2/Content Shell.app
//out/gn2/Content Shell.app/Contents/Frameworks/Content Shell
Framework.framework
//out/gn2/Content Shell.app/Contents/Frameworks/Content Shell Helper.app
//out/gn2/Content Shell.app/Contents/Info.plist
//out/gn2/Content Shell.app/Contents/MacOS/Content Shell
//out/gn2/Content Shell.app/Contents/Resources/app.icns
On 2016/04/15 22:14:35, Robert Sesek wrote: > On 2016/04/15 22:05:01, Dirk Pranke wrote: > > I'm a bit confused by this change. I can see that you're creating a > 'bar.bundle' > > rule, but what do you mean by "the dependent bundle can be specified as a > > sources"? > > > > I feel like either I don't understand what you're trying to do, or we've got > the > > dependencies wrong somehow, because creating a phony target in ninja shouldn't > > affect the list of sources or how we specify dependencies. > > This is needed for the way Content Shell and Chromium work. Each have a > .framework bundle that then needs to be a bundle_data of the .app bundle. > Currently the outputs of a bundle only list the individual files that go into > the bundle, and it doesn't treat the fully packaged bundle as a discrete output. > It is necessary to treat the output of create_bundle as a single unit so that it > can be copied into another bundle. > > E.g., before this change: > > outputs: > //out/gn2/Content Shell.app/Contents/Frameworks/Content Shell > Framework.framework > //out/gn2/Content Shell.app/Contents/Frameworks/Content Shell Helper.app > //out/gn2/Content Shell.app/Contents/Info.plist > //out/gn2/Content Shell.app/Contents/MacOS/Content Shell > //out/gn2/Content Shell.app/Contents/Resources/app.icns > > With this change: > > outputs: > //out/gn2/Content Shell.app > //out/gn2/Content Shell.app/Contents/Frameworks/Content Shell > Framework.framework > //out/gn2/Content Shell.app/Contents/Frameworks/Content Shell Helper.app > //out/gn2/Content Shell.app/Contents/Info.plist > //out/gn2/Content Shell.app/Contents/MacOS/Content Shell > //out/gn2/Content Shell.app/Contents/Resources/app.icns I see. Still, that seems confusing to list both a directory and (part of) its contents as outputs. Is there some way we can make this work with deps instead of outputs? I've lost track of which things are being tracked inside GN and which things are in the templates. Apologies if my question doesn't make sense; all of this stuff is still pretty fuzzy to me.
On 2016/04/15 22:29:31, Dirk Pranke wrote: > On 2016/04/15 22:14:35, Robert Sesek wrote: > > On 2016/04/15 22:05:01, Dirk Pranke wrote: > > > I'm a bit confused by this change. I can see that you're creating a > > 'bar.bundle' > > > rule, but what do you mean by "the dependent bundle can be specified as a > > > sources"? > > > > > > I feel like either I don't understand what you're trying to do, or we've got > > the > > > dependencies wrong somehow, because creating a phony target in ninja > shouldn't > > > affect the list of sources or how we specify dependencies. > > > > This is needed for the way Content Shell and Chromium work. Each have a > > .framework bundle that then needs to be a bundle_data of the .app bundle. > > Currently the outputs of a bundle only list the individual files that go into > > the bundle, and it doesn't treat the fully packaged bundle as a discrete > output. > > It is necessary to treat the output of create_bundle as a single unit so that > it > > can be copied into another bundle. > > > > E.g., before this change: > > > > outputs: > > //out/gn2/Content Shell.app/Contents/Frameworks/Content Shell > > Framework.framework > > //out/gn2/Content Shell.app/Contents/Frameworks/Content Shell Helper.app > > //out/gn2/Content Shell.app/Contents/Info.plist > > //out/gn2/Content Shell.app/Contents/MacOS/Content Shell > > //out/gn2/Content Shell.app/Contents/Resources/app.icns > > > > With this change: > > > > outputs: > > //out/gn2/Content Shell.app > > //out/gn2/Content Shell.app/Contents/Frameworks/Content Shell > > Framework.framework > > //out/gn2/Content Shell.app/Contents/Frameworks/Content Shell Helper.app > > //out/gn2/Content Shell.app/Contents/Info.plist > > //out/gn2/Content Shell.app/Contents/MacOS/Content Shell > > //out/gn2/Content Shell.app/Contents/Resources/app.icns > > I see. > > Still, that seems confusing to list both a directory and (part of) its contents > as outputs. > > Is there some way we can make this work with deps instead of outputs? I've lost > track > of which things are being tracked inside GN and which things are in the > templates. > > Apologies if my question doesn't make sense; all of this stuff is still pretty > fuzzy to me. I don't think so because nothing actually lists the root bundle directory as a build edge. I was listing the framework as a deps in the bundle_data but GN complains that nothing generates the output.
Ah, I think I get it now. I missed that this was also telling GN that the directory was an output. I think this lgtm, but it's probably safer to have brettw look at it as well.
https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.cc File tools/gn/bundle_data.cc (right): https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.cc#new... tools/gn/bundle_data.cc:119: const SourceDir& root_out_dir = settings->build_settings()->build_dir(); This variable should be called build_dir or root_build_dir. The out_dir is different (it's per-toolchain). https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.cc#new... tools/gn/bundle_data.cc:121: base::StringPiece(root_dir()).substr(root_out_dir.value().length()); I'm confused by this code. The root_dir() returns a string. I don't see what enforces this is anything in particular. If that's a directory inside the root_build_dir, it should probably be of type SourceDir. I also think this would be better expressed as a RebasePath call (filesystem_utils.h) to avoid error-prone manual path manipulation. https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.h File tools/gn/bundle_data.h (right): https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.h#newc... tools/gn/bundle_data.h:71: SourceFile GetBundleRootDirOutput(const Settings* settings) const; Can this return a SourceDir instead? The thing you're returning is really a directory so I think we should be semantically correct here (will need to ensure it ends in a slash before you construct it).
https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.cc File tools/gn/bundle_data.cc (right): https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.cc#new... tools/gn/bundle_data.cc:119: const SourceDir& root_out_dir = settings->build_settings()->build_dir(); On 2016/04/18 20:16:14, brettw wrote: > This variable should be called build_dir or root_build_dir. The out_dir is > different (it's per-toolchain). Done. https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.cc#new... tools/gn/bundle_data.cc:121: base::StringPiece(root_dir()).substr(root_out_dir.value().length()); On 2016/04/18 20:16:14, brettw wrote: > I'm confused by this code. The root_dir() returns a string. I don't see what > enforces this is anything in particular. If that's a directory inside the > root_build_dir, it should probably be of type SourceDir. The CreateBundleTargetGenerator extracts these from the target and verifies that they're in the output directory. Are you suggesting changing these values from strings to SourceDirs (which would be a change larger than just what's being done here)? > I also think this would be better expressed as a RebasePath call > (filesystem_utils.h) to avoid error-prone manual path manipulation. I looked at RebasePath and I wasn't able to find a way to make it work. The bundle's root_dir has to be in build_dir, but it could be either of these two values, for example: out/gn/Foo.app/Contents out/gn/Foo.app And in either case, the the bundle's root output directory should be: out/gn/Foo.app Since that is the final packaged output of the create_bundle step. Conceptually we just want the first directory name inside the build_dir, which I don't think RebasePath helps with. https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.h File tools/gn/bundle_data.h (right): https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.h#newc... tools/gn/bundle_data.h:71: SourceFile GetBundleRootDirOutput(const Settings* settings) const; On 2016/04/18 20:16:14, brettw wrote: > Can this return a SourceDir instead? The thing you're returning is really a > directory so I think we should be semantically correct here (will need to ensure > it ends in a slash before you construct it). Is it acceptable to convert a SourceDir to an OutputFile? There isn't a direct ctor for that so it would have to be indirected through .value() conversion. Conceptually this may actually be a SourceFile, though. This CL basically treating the output of a create_bundle as a single unit (a "source file") so that it can be used as a source elsewhere.
ping
LGTM with comments addressed. https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.cc File tools/gn/bundle_data.cc (right): https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.cc#new... tools/gn/bundle_data.cc:121: base::StringPiece(root_dir()).substr(root_out_dir.value().length()); On 2016/04/19 15:37:51, Robert Sesek wrote: > The CreateBundleTargetGenerator extracts these from the target and verifies that > they're in the output directory. Are you suggesting changing these values from > strings to SourceDirs (which would be a change larger than just what's being > done here)? Yes, I think these should be changed (unless there's something I'm missing) but that doesn't need to block the immediate work we're doing here. > > I also think this would be better expressed as a RebasePath call > > (filesystem_utils.h) to avoid error-prone manual path manipulation. > > I looked at RebasePath and I wasn't able to find a way to make it work. The > bundle's root_dir has to be in build_dir, but it could be either of these two > values, for example: > > out/gn/Foo.app/Contents > out/gn/Foo.app I was suggesting that just the bundle_root_relative computation be replaced by RebasePath(root_dir(), root_out_dir) and then keep the code below with the first_component computation. https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.h File tools/gn/bundle_data.h (right): https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.h#newc... tools/gn/bundle_data.h:71: SourceFile GetBundleRootDirOutput(const Settings* settings) const; On 2016/04/19 15:37:51, Robert Sesek wrote: > On 2016/04/18 20:16:14, brettw wrote: > > Can this return a SourceDir instead? The thing you're returning is really a > > directory so I think we should be semantically correct here (will need to > ensure > > it ends in a slash before you construct it). > > Is it acceptable to convert a SourceDir to an OutputFile? There isn't a direct > ctor for that so it would have to be indirected through .value() conversion. > > Conceptually this may actually be a SourceFile, though. This CL basically > treating the output of a create_bundle as a single unit (a "source file") so > that it can be used as a source elsewhere. I can kind of see that. Can the comment above the function explain what you just wrote here about why it's a SourceFile? https://codereview.chromium.org/1892243002/diff/1/tools/gn/ninja_create_bundl... File tools/gn/ninja_create_bundle_target_writer.cc (right): https://codereview.chromium.org/1892243002/diff/1/tools/gn/ninja_create_bundl... tools/gn/ninja_create_bundle_target_writer.cc:120: out_ << "build "; Can you write a comment here mentioning what this phony target is for?
https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.cc File tools/gn/bundle_data.cc (right): https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.cc#new... tools/gn/bundle_data.cc:121: base::StringPiece(root_dir()).substr(root_out_dir.value().length()); On 2016/04/21 19:14:00, brettw wrote: > On 2016/04/19 15:37:51, Robert Sesek wrote: > > The CreateBundleTargetGenerator extracts these from the target and verifies > that > > they're in the output directory. Are you suggesting changing these values from > > strings to SourceDirs (which would be a change larger than just what's being > > done here)? > > Yes, I think these should be changed (unless there's something I'm missing) but > that doesn't need to block the immediate work we're doing here. I'll do this in a follow-up CL. > > > > I also think this would be better expressed as a RebasePath call > > > (filesystem_utils.h) to avoid error-prone manual path manipulation. > > > > I looked at RebasePath and I wasn't able to find a way to make it work. The > > bundle's root_dir has to be in build_dir, but it could be either of these two > > values, for example: > > > > out/gn/Foo.app/Contents > > out/gn/Foo.app > > I was suggesting that just the bundle_root_relative computation be replaced by > RebasePath(root_dir(), root_out_dir) and then keep the code below with the > first_component computation. Ah, done. https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.h File tools/gn/bundle_data.h (right): https://codereview.chromium.org/1892243002/diff/1/tools/gn/bundle_data.h#newc... tools/gn/bundle_data.h:71: SourceFile GetBundleRootDirOutput(const Settings* settings) const; On 2016/04/21 19:14:00, brettw wrote: > On 2016/04/19 15:37:51, Robert Sesek wrote: > > On 2016/04/18 20:16:14, brettw wrote: > > > Can this return a SourceDir instead? The thing you're returning is really a > > > directory so I think we should be semantically correct here (will need to > > ensure > > > it ends in a slash before you construct it). > > > > Is it acceptable to convert a SourceDir to an OutputFile? There isn't a direct > > ctor for that so it would have to be indirected through .value() conversion. > > > > Conceptually this may actually be a SourceFile, though. This CL basically > > treating the output of a create_bundle as a single unit (a "source file") so > > that it can be used as a source elsewhere. > > I can kind of see that. Can the comment above the function explain what you just > wrote here about why it's a SourceFile? Done. https://codereview.chromium.org/1892243002/diff/1/tools/gn/ninja_create_bundl... File tools/gn/ninja_create_bundle_target_writer.cc (right): https://codereview.chromium.org/1892243002/diff/1/tools/gn/ninja_create_bundl... tools/gn/ninja_create_bundle_target_writer.cc:120: out_ << "build "; On 2016/04/21 19:14:00, brettw wrote: > Can you write a comment here mentioning what this phony target is for? Done.
The CQ bit was checked by rsesek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1892243002/#ps40001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892243002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892243002/40001
Message was sent while issue was closed.
Description was changed from ========== Write a phony target for the top-level directory of a create_bundle target. This is needed so that if a create_bundle is used as a bundle_data in another bundle, the dependent bundle can be specified as a sources. BUG=297668 R=brettw@chromium.org ========== to ========== Write a phony target for the top-level directory of a create_bundle target. This is needed so that if a create_bundle is used as a bundle_data in another bundle, the dependent bundle can be specified as a sources. BUG=297668 R=brettw@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Write a phony target for the top-level directory of a create_bundle target. This is needed so that if a create_bundle is used as a bundle_data in another bundle, the dependent bundle can be specified as a sources. BUG=297668 R=brettw@chromium.org ========== to ========== Write a phony target for the top-level directory of a create_bundle target. This is needed so that if a create_bundle is used as a bundle_data in another bundle, the dependent bundle can be specified as a sources. BUG=297668 R=brettw@chromium.org Committed: https://crrev.com/a23feb50e97c016318d5a859609f8f6a139f0ed5 Cr-Commit-Position: refs/heads/master@{#389101} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a23feb50e97c016318d5a859609f8f6a139f0ed5 Cr-Commit-Position: refs/heads/master@{#389101} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
