Bug 89450

Created attachment 114053 [details] [review]
Fix cmake build system bug not generating versioned library name in case LT_REVISION is zero

Created attachment 114054 [details] [review]
Fix cmake 3.1.3 policy CMP0026 warning

Created attachment 114055 [details] [review]
Fix cmake 3.1.3 policy CMP0053 warning

Created attachment 114056 [details] [review]
Fix cmake 3.1.3 policy CMP0054 warning

Comment on attachment 114053 [details] [review]
Fix cmake build system bug not generating versioned library name in case LT_REVISION is zero

Review of attachment 114053 [details] [review]:
-----------------------------------------------------------------

Yes

Comment on attachment 114054 [details] [review]
Fix cmake 3.1.3 policy CMP0026 warning

Review of attachment 114054 [details] [review]:
-----------------------------------------------------------------

I'd prefer a commit message that gives more of a clue what is changing to a reader not following the link, perhaps something like this as a replacement for the first line:

cmake: stop using deprecated LOCATION property (policy CMP0026)

but the actual change looks fine.

Comment on attachment 114055 [details] [review]
Fix cmake 3.1.3 policy CMP0053 warning

Review of attachment 114055 [details] [review]:
-----------------------------------------------------------------

Again, I'd prefer the first line of the commit message to give more clue what's going on here and why. If I'm understanding it correctly, perhaps it's something like this:

cmake: @VAR@ substitutions in set() are deprecated, use string(CONFIGURE) instead (policy CMP0053)

Comment on attachment 114056 [details] [review]
Fix cmake 3.1.3 policy CMP0054 warning

Review of attachment 114056 [details] [review]:
-----------------------------------------------------------------

cmake: opt-in to not implicitly expanding variables in if() (policy CMP0054)

or something?

Comment on attachment 114053 [details] [review]
Fix cmake build system bug not generating versioned library name in case LT_REVISION is zero

applied

Comment on attachment 114054 [details] [review]
Fix cmake 3.1.3 policy CMP0026 warning

applied with modified commit message

Comment on attachment 114055 [details] [review]
Fix cmake 3.1.3 policy CMP0053 warning

applied with modified commit text

Comment on attachment 114056 [details] [review]
Fix cmake 3.1.3 policy CMP0054 warning

applied with modified commit text

All are applied. Is there anything more to do here?

This one http://cgit.freedesktop.org/~smcv/dbus/commit/?h=wip-dbussocket&id=73c6219e087d00bcf97d5e94de252a7631a29180 belongs to cmake support ?

(In reply to Simon McVittie from comment #13)
> All are applied. Is there anything more to do here?

There are still a few issues like the missing '=' in the printed information shown below:


 D-BUS 1.9.11                               
 ===========  


I will add related patches soon.

(In reply to Ralf Habacker from comment #14)
> This one
> http://cgit.freedesktop.org/~smcv/dbus/commit/?h=wip-
> dbussocket&id=73c6219e087d00bcf97d5e94de252a7631a29180 belongs to cmake
> support ?

Yes, or something... the build regressed on cmake 3.0, which is what I have, as a result of the changes seen on this bug.

That branch isn't fully tidied up yet, but if you're OK with that commit, feel free to (consider it to be part of this bug and) apply it to master.

Of the commits in that branch, "Rename _dbus_full_duplex_pipe() to more descriptive name _dbus_socketpair()" is good to apply whenever you want, "Fix assorted compiler warnings on Windows" would ideally be multiple commits but I've been busy with non-D-Bus things (and it would be OK to apply as-is if you approve), and "cmake: only set CMP0053, CMP0054 on CMake >= 3.1" is really this bug.

Subsequent commits, including the fixed version of your "Use typedef DBusSocket for sockets fd's to avoid conversion warnings", shouldn't be applied unless all of them are. I'll say more about that on Bug #89444.

(In reply to Ralf Habacker from comment #15)
> I will add related patches soon.
applied to master.

(In reply to Simon McVittie from comment #16)

> Of the commits in that branch, "Rename _dbus_full_duplex_pipe() to more
> descriptive name _dbus_socketpair()" is good to apply whenever you want,
applied to master

> "Fix assorted compiler warnings on Windows" would ideally be multiple
> commits but I've been busy with non-D-Bus things (and it would be OK to
> apply as-is if you approve)
rebased and applied to master

> and "cmake: only set CMP0053, CMP0054 on CMake
> >= 3.1" is really this bug.
applied to master