Bug 16620 – [GTK] Autotools make dist and make check support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Jan Alonzo]

Support for make dist and make check targets. There is currently no support for these targets and it would be nice to have them.

[Alp Toker]

I would say this is a characteristic of autotools. The correct way to avoid littering the source tree, as you pointed out yourself, is to build outside of the source tree.

Please re-open the bug if you disagree.

[Pierre-Luc Beaudoin]

I believe make dist and make check are needed to build inside the scratchbox.  That's why I did pass some time on it without much success (lack of knowledge on autotools from me).

[Alp Toker]

(In reply to comment #2)
> I believe make dist and make check are needed to build inside the scratchbox. 
> That's why I did pass some time on it without much success (lack of knowledge
> on autotools from me).
> 

Sorry, ignore comment #1. It was intended for bug #16619 and not bug #16620. Re-opening.

[Jan Alonzo]

Created attachment 19960 [details]
autotools dist support for webkit/gtk

The patch fixes the 'dist' target for webkit/gtk autotools. The patch uses 'find' to look for header files and other EXTRA files for distribution. 

Cheers.

[Alp Toker]

(In reply to comment #4)
> Created an attachment (id=19960) [edit]
> autotools dist support for webkit/gtk
> 
> The patch fixes the 'dist' target for webkit/gtk autotools. The patch uses
> 'find' to look for header files and other EXTRA files for distribution. 
> 
> Cheers.
> 

Sorry this took a while to get looked at.

This is an important patch but I'm slightly concerned about the 'find' parts. Will this include the extra files I have lying around in my working directory at make dist time?

I think it's important to list all distributed files explicitly. I wonder how much we can re-use the sources lists for this.

If you can submit the most obvious parts of this patch, perhaps in two or three smaller parts that'd help me to trickle the changes in. They don't have to fix make dist in one go as long as they move us in the right direction.

[Jan Alonzo]

Hi Alp,

Thanks for the feedback. Is it ok to use find for non-source/header files (e.g, .idl, .gperf, .css, etc..)?

cheers

[Alp Toker]

(In reply to comment #6)
> Hi Alp,
> 
> Thanks for the feedback. Is it ok to use find for non-source/header files (e.g,
> .idl, .gperf, .css, etc..)?
> 

find is bad for these cases too. Apart from the IDLs I don't think the lists of files will be too big though, right?

If the IDL list is too long, you can skip that for now and get the easy parts fixed in this patch. Your call.

[Alp Toker]

Comment on attachment 19960 [details]
autotools dist support for webkit/gtk

Clearing flag for now based on my previous comments. It's important to get this done though.

[Jan Alonzo]

Created attachment 20505 [details]
[1/3] Cleanup autotools build scripts

This patch fixes the following the clean target, removed unnecessary variables and unwanted spaces and favouring use of @abs_top_builddir@ instead of of $(CURDIR)

[Jan Alonzo]

Created attachment 20506 [details]
[2/3] Add header files in webkit/gtk sources

This patch adds the header files required to build the webkit/gtk port

[Jan Alonzo]

Created attachment 20507 [details]
[3/3] Add auxillary files, scripts to build webkit/gtk

This patch adds the required scripts and other files necessary to make a webkit/gtk distribution.

Alp: I'm using find to gather all the IDLs hence removing traces of IDL_BINDINGS. It's only for building a distribution. Sources from IDLs won't be built unless it's in webcore_built_sources.

Cheers.

[Jan Alonzo]

Also, where do you want me to put the Changelog? For each patch or just one changelog for the series?

Thanks.

[Alp Toker]

Comment on attachment 20505 [details]
[1/3] Cleanup autotools build scripts

Looks OK (except for this whitespace addition)

-lib_LIBRARIES :=
+lib_LIBRARIES :=

[Alp Toker]

(In reply to comment #13)
> (From update of attachment 20505 [details] [edit])
> Looks OK (except for this whitespace addition)
> 
> -lib_LIBRARIES :=
> +lib_LIBRARIES := 
> 

Jan, were you able to reproduce the warnings I passed on to you with the first patch? I'm waiting on that now

[Alp Toker]

Jan, can you also mention whether putting the codegen commands on one line instead of line breaking is a build fix or a readability fix?

Either way, it's fine, but either way it's worth mentioning the reason for the change.

Thanks

[Jan Alonzo]

Created attachment 20683 [details]
Cleanup build scripts - update

Updated patch 1 of this series to match ToT and fixed generate-bindings.pl warnings.

[Jan Alonzo]

(In reply to comment #15)
> Jan, can you also mention whether putting the codegen commands on one line
> instead of line breaking is a build fix or a readability fix?
> 
> Either way, it's fine, but either way it's worth mentioning the reason for the
> change.
> 
> Thanks

Btw, it's a readability fix

[Jan Alonzo]

Created attachment 20686 [details]
[2/3] Updated add headers patch

Updated patch 2 to match ToT. Also, remove webkitgtk_headers and sorted _sources as well.

[Jan Alonzo]

Created attachment 20687 [details]
[3/3] Updated patch

Updated patch to match ToT and to apply with patch 2 changes

[Jan Alonzo]

Comment on attachment 20686 [details]
[2/3] Updated add headers patch

Clearing review flag as this is already obsolete.

[Jan Alonzo]

Comment on attachment 20687 [details]
[3/3] Updated patch

Clearing review flag as this is already obsolete

[Darin Adler]

What's the status on this bug? There's a patch here marked review+ -- does that need to be landed or is it obsolete too?

[Jan Alonzo]

Darin: The r+'d patch was already landed. Should we split this?

[Christian Dywan]

Comment on attachment 20683 [details]
Cleanup build scripts - update

Clearing review flag for the sake of reducing confusion about the state of this bug.

[Jan Alonzo]

Closing this for now. I'll file the rest of the patches in a separate bug when they're ready.

[Alp Toker]

Landed (with several updates and fixes) in r35658.