Bug 139387 – [GTK] Generate the make dist manifest from a CMake template file

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

[Michael Catanzaro]

If we generate the install manifest from a CMake template file, we can use CMake variables in the manifest. This would be useful for bug #139369.

[Carlos Garcia Campos]

Our manifest is not for installation, but to decide what files to include in the tarball for releasing.

[Michael Catanzaro]

Created attachment 242792 [details]
Patch

[Michael Catanzaro]

(In reply to comment #1)
> Our manifest is not for installation, but to decide what files to include in
> the tarball for releasing.

Yes, but I need more control over what files are included in the tarball for releasing to fix bug #139369.

[Carlos Garcia Campos]

The thing is that we already have a custom variable substitution mechanism for the manifest. I think we should either add the abi-version variable to the existing mechanism, or remove the custom code an use CMake for the other variables. Martin, you wrote that code, what do you think?

[Michael Catanzaro]

(In reply to comment #4)
> The thing is that we already have a custom variable substitution mechanism
> for the manifest. I think we should either add the abi-version variable to
> the existing mechanism, or remove the custom code an use CMake for the other
> variables. Martin, you wrote that code, what do you think?

OK. make-dist.py is passed the right sourcedir and builddir when it is called, which it needs anyway, so it also incidentally replaces $source and $build in the manifest. For it to be able to replace other variables, values would need to be passed as arguments to the script, which doesn't seem like a good approach. $source is never used in the manifest, and $build is only used on the two lines that I modify in bug #139369. So I would generally prefer to replace $build with ${CMAKE_BINARY_DIR} on those two lines and remove the code in make-dist.py that expands $build and $source.

But, on the other hand, I see that our API version is hardcoded in make-dist.py. There is no value in using a CMake variable for it in the manifest but not in make-dist.py, so we should add an argument to the script for it regardless (or just hardcode it in both places, but let's be better than that). This would obviate the need to use CMake variables in the manifest (at least for now, but I can't see why else we would need it in the future).

[Martin Robinson]

(In reply to comment #4)
> The thing is that we already have a custom variable substitution mechanism
> for the manifest. I think we should either add the abi-version variable to
> the existing mechanism, or remove the custom code an use CMake for the other
> variables. Martin, you wrote that code, what do you think?

It does seem that using the CMake preprocessor could simplify the script quite a bit. I think this patch should also rip out the variable replacement code in the script and use the CMake preprocessor for everything though.

[Michael Catanzaro]

Like I said in comment #5, I no longer see value in using a CMake template for the manifest so long as we still hardcode our API version in make-dist.py. We should either pass the API version as an argument to make-dist.py (in which case there is no point in using a CMake template for the manifest), or else make make-dist.py itself a CMake template. I don't see any significant advantages or disadvantages to either approach; using CMake templates makes the make-dist script shorter, which I prefer, so I will upload a patch for that, but it also makes the script less flexible in case you want to play with calling the script manually (as I have removed the srcdir and builddir arguments), though I don't see why you would want to do that.

[Martin Robinson]

(In reply to comment #7)
> Like I said in comment #5, I no longer see value in using a CMake template
> for the manifest so long as we still hardcode our API version in
> make-dist.py. We should either pass the API version as an argument to
> make-dist.py (in which case there is no point in using a CMake template for
> the manifest), or else make make-dist.py itself a CMake template. I don't
> see any significant advantages or disadvantages to either approach; using
> CMake templates makes the make-dist script shorter, which I prefer, so I
> will upload a patch for that, but it also makes the script less flexible in
> case you want to play with calling the script manually (as I have removed
> the srcdir and builddir arguments), though I don't see why you would want to
> do that.
 

It should be possible to get rid of the hard-coded version by simply looking for the latest file with the pattern webkit2gtk-*.pc.

[Michael Catanzaro]

(In reply to comment #8)
> It should be possible to get rid of the hard-coded version by simply looking
> for the latest file with the pattern webkit2gtk-*.pc.

That's not really any better than just passing it as an argument to the script. I'll do that in a separate bug since it's not necessary here.

I do have a patch to turn make-dist.py into a CMake template, but it's larger than I expected and I don't really like it, so I will just turn the manifest into a template and remove the variable replacement code from make-dist.py (not very much code) like you requested.

[Michael Catanzaro]

Created attachment 244647 [details]
Patch

[Michael Catanzaro]

(In reply to comment #9)
> That's not really any better than just passing it as an argument to the
> script. I'll do that in a separate bug since it's not necessary here.

Actually, I'll leave it be and get back to real work, because the API version is only used in the script to call pkg-config to come up with a default version to use if --version is not passed to the script (which is not how we use it).

[Martin Robinson]

Comment on attachment 244647 [details]
Patch

Okay. Thanks!

[WebKit Commit Bot]

Comment on attachment 244647 [details]
Patch

Clearing flags on attachment: 244647

Committed r178672: <http://trac.webkit.org/changeset/178672>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.