Bug 214696 – Submission failure with "make: *** No rule to make target `installsrc'. Stop"

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Ryan Hostetler]

Submission as a single project fails with:

stderr:
make: *** No rule to make target `installsrc'.  Stop.

[Ryan Hostetler]

Created attachment 405066 [details]
Patch

[Tim Horton]

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?

[Ryan Hostetler]

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.

[Ryan Hostetler]

Created attachment 405071 [details]
Patch

[Alexey Proskuryakov]

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.

[Ryan Hostetler]

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.

[Ryan Hostetler]

Created attachment 405075 [details]
Patch

[Jonathan Bedard]

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

[Ryan Hostetler]

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."

[Tim Horton]

(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!?

[Darin Adler]

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.

[Keith Rollin]

(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.

[Keith Rollin]

Is this Bug related to <rdar://62268104>?

[Ryan Hostetler]

(In reply to Keith Rollin from comment #13)
> Is this Bug related to <rdar://62268104>?

Yes. Is there a good way to associate it?

[Tim Horton]

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

[Tim Horton]

(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)

[Keith Rollin]

(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.

[Ryan Hostetler]

Created attachment 405114 [details]
Patch

[Ryan Hostetler]

Created attachment 405117 [details]
Patch

[Ryan Hostetler]

Created attachment 405119 [details]
Patch

[Alexey Proskuryakov]

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.

[Ryan Hostetler]

(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.

[Alexey Proskuryakov]

I don't expect the content to be the same. Have you compared what goes into SRCROOT before and after?

[Keith Rollin]

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?

[Keith Rollin]

> 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.

[Ryan Hostetler]

Created attachment 405896 [details]
Patch

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Ryan Hostetler]

Created attachment 410374 [details]
Patch

[EWS]

Committed r267949: <https://trac.webkit.org/changeset/267949>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410374 [details].