Submission as a single project fails with: stderr: make: *** No rule to make target `installsrc'. Stop.
Created attachment 405066 [details] Patch
Comment on attachment 405066 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405066&action=review > Makefile:42 > +clean: Seems like we now have two `clean` targets?
Comment on attachment 405066 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405066&action=review >> Makefile:42 >> +clean: > > Seems like we now have two `clean` targets? Yes, we need a "clean:\n\t" as a fall-through for any projects not listed in MODULES.
Created attachment 405071 [details] Patch
Comment on attachment 405071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405071&action=review > Makefile:34 > + ditto Makefile $(SRCROOT)/Makefile > + ditto Source $(SRCROOT)/Source > + mkdir -p $(SRCROOT)/WebKitLibraries Is modifying SRCROOT normal? It certainly sounds counter-intuitive. Also, the flag is unnecessary in mkdir -p, and should be removed - first, SRCROOT always exists, and second, the preceding commands would have failed if it didn't.
Comment on attachment 405071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405071&action=review >> Makefile:34 >> + mkdir -p $(SRCROOT)/WebKitLibraries > > Is modifying SRCROOT normal? It certainly sounds counter-intuitive. > > Also, the flag is unnecessary in mkdir -p, and should be removed - first, SRCROOT always exists, and second, the preceding commands would have failed if it didn't. Good catch on '-p' I'll remove it! During project submission phase we fill the SRCROOT with content required for building, we do this so we don't submit the entire repository.
Created attachment 405075 [details] Patch
Comment on attachment 405075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405075&action=review > Makefile:42 > +clean: We already have a clean phase, line 27
Comment on attachment 405075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405075&action=review >> Makefile:42 >> +clean: > > We already have a clean phase, line 27 You're right, see the comment to Tim Horton: "Yes, we need a "clean:\n\t" as a fall-through for any projects not listed in MODULES."
(In reply to Ryan Hostetler from comment #9) > Comment on attachment 405075 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405075&action=review > > >> Makefile:42 > >> +clean: > > > > We already have a clean phase, line 27 > > You're right, see the comment to Tim Horton: > "Yes, we need a "clean:\n\t" as a fall-through for any projects not listed > in MODULES." I admit I am not a Make expert, but the comment doesn't make sense to me. How are the phases different? They don't have different dependencies or anything, so should run at the same time. I'm not even sure what happens if you have two identical targets!?
Comment on attachment 405075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405075&action=review >>>> Makefile:42 >>>> +clean: >>> >>> We already have a clean phase, line 27 >> >> You're right, see the comment to Tim Horton: >> "Yes, we need a "clean:\n\t" as a fall-through for any projects not listed in MODULES." > > I admit I am not a Make expert, but the comment doesn't make sense to me. How are the phases different? They don't have different dependencies or anything, so should run at the same time. I'm not even sure what happens if you have two identical targets!? I am pretty expert on make and it certainly does not make sense to me.
(In reply to Darin Adler from comment #11) > > I am pretty expert on make and it certainly does not make sense to me. I think you can specify a target multiple times to establish dependencies, but I think it makes sense to have only one target specify the build rules. Which means that it doesn't seem to make sense to have a bare "clean:" that doesn't establish any new dependencies or provide rules. If one types `make clean`, our existing first rule will get executed.
Is this Bug related to <rdar://62268104>?
(In reply to Keith Rollin from comment #13) > Is this Bug related to <rdar://62268104>? Yes. Is there a good way to associate it?
3 things: 1) mention the radar # in a comment, 2) set the InRadar keyword 3) insert the radar # below the bugzilla URL in the ChangeLog
(In reply to Tim Horton from comment #15) > 3 things: 1) mention the radar # in a comment, 2) set the InRadar keyword 3) > insert the radar # below the bugzilla URL in the ChangeLog (as a URL; see other ChangeLog entries)
(In reply to Tim Horton from comment #15) > 3 things: 1) mention the radar # in a comment, 2) set the InRadar keyword 3) > insert the radar # below the bugzilla URL in the ChangeLog And add webkit-bug-importer to the CC list. And add the Bugzilla ID to the radar title.
Created attachment 405114 [details] Patch
Created attachment 405117 [details] Patch
Created attachment 405119 [details] Patch
Comment on attachment 405119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405119&action=review > Makefile:34 > + ditto Makefile $(SRCROOT)/Makefile > + ditto Source $(SRCROOT)/Source I still think that this needs to recurse to actual projects. Even though I questioned the content of installhdrs/install offline, it's not clear to me if they can be omitted completely. Are you doing end-to-end testing? I don't think that testing is blocked, maybe we should discuss it if you don't have a way to test now.
(In reply to Alexey Proskuryakov from comment #21) > Comment on attachment 405119 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405119&action=review > > > Makefile:34 > > + ditto Makefile $(SRCROOT)/Makefile > > + ditto Source $(SRCROOT)/Source > > I still think that this needs to recurse to actual projects. > > Even though I questioned the content of installhdrs/install offline, it's > not clear to me if they can be omitted completely. Are you doing end-to-end > testing? I don't think that testing is blocked, maybe we should discuss it > if you don't have a way to test now. If we recurse the projects we end up copying the same content in more steps. We have support for end to end testing.
I don't expect the content to be the same. Have you compared what goes into SRCROOT before and after?
It's not apparent to me what these changes are for or why they're being made. It will probably not be apparent to future generations, either, so good explanatory comments (or directions to documentation) seem necessary. For instance, the description of this bug is merely "No rule to make target `installsrc'". OK, so you added such a target. I can follow that much. But why are you updating the "clean" target? My first guess would be that it would clean up whatever was done by "installsrc". But your change to "clean" effectively stubs it out under certain conditions. So it can't be the case that "clean" is cleaning up after "installsrc". So what's the intent here? And those "certain conditions" seem to involve building the WebKit project. What effect does your change have on the current process of building WebKit?
> OK, so you added such a target. I can follow that much. Though, actually, it's not clear to me why we need such a target now. We haven't needed one before. The reason for needing it now might be a good one, but it's not apparent what that reason is from the patch -- it's not self-apparent. So that reason will need to be made clear somehow and somewhere.
Created attachment 405896 [details] Patch
Comment on attachment 405896 [details] Patch Marking r- until there is evidence that copying everything is OK, or the patch is updated to recurse into modules and perform installsrc in each project.
As discovered while discussing offline, the EXCLUDED_INSTALLSRC_SUBDIRECTORY_PATTERNS rule in libwebrtc was added to fix the build, so bypassing it will likely break the build.
Created attachment 410374 [details] Patch
Committed r267949: <https://trac.webkit.org/changeset/267949> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410374 [details].