search

ubports / ubuntu-ui-toolkit

Moved to https://gitlab.com/ubports/core/lomiri-ui-toolkit
https://gitlab.com/ubports/core/lomiri-ui-toolkit
GNU Lesser General Public License v3.0
13 stars 21 forks source link

Adapt to QtPim API changes #20

Closed z3ntu closed 5 years ago

z3ntu commented 6 years ago

Fixes #9

~~Feel free to split up the commits, take half of it or whatever Commit message should really be changed before merging at least.~~

z3ntu commented 6 years ago

I've split up the commits now. Content is the same.

vanyasem commented 6 years ago

You probably also need this https://github.com/ubports/ubuntu-ui-toolkit/commit/4f2691b336dedbbeee66434cfafce211170e659a#diff-d837a1c17de0268bcea321239ddbed47

mardy commented 5 years ago

@dobey, please notice that these are three different commits; if they are merged with the "Rebase and merge" option, they'll land as separate commits. I can't talk about the other two commits, but the one about QtPim is a needed fix which we already have in the xenial branch (without it, the package fails to build against a recent QtPim).

dobey commented 5 years ago

@mardy I wasn't suggesting they weren't separate commits. I was suggesting they should be separate branches with separate PRs, because they fix separate things, and that what they're fixing needs to be documented in some way (preferably with an issue being fixed), and they need tests.

z3ntu commented 5 years ago

"Add option to disable building of docs" just adds an option for qmake not to build the docs.

"Fix "Missing sentinel in function call" warning" fixes a compilation warning that I got when building ubuntu-ui-toolkit on Alpine.

And the first commit was explained by @mardy.

I can split the PR into 3 PRs if that's really wanted :)

dobey commented 5 years ago

Well it seems that the "compiles with newer qtpim" doesn't actually fix it, as you can see at the failed CI check log.

z3ntu commented 5 years ago

Probably because the CI has an older version of qtpim installed that doesn't need that change.

vanyasem commented 5 years ago

It fails to compile because of that: https://github.com/ubports/ubuntu-ui-toolkit/pull/20#issuecomment-427595220. You need to specify that there's no need to install older qtpim in debian/control

z3ntu commented 5 years ago

@vanyasem I added that but it still fails to build.

The following packages have unmet dependencies:

 pbuilder-satisfydepends-dummy : Depends: qtpim5-dev (>= 5.0~git20171109~0bd985b) but it is not going to be installed
dobey commented 5 years ago

It fails, because the newer qtpim is not packaged in bionic. I think we should close this PR and open issues for any problems there are, and we'd need to do a concerted effort to get latest qtpim into bionic builds, along with the necessary changes for qtpim into bionic branches. And we need proper testing for things like this. For example, I don't know what warning is fixed by adding a char* cast to a NULL, but it feels wrong looking at that change.

z3ntu commented 5 years ago

The exact warning without the patch is

ucurihandler.cpp: In constructor 'UCUriHandler::UCUriHandler()':
ucurihandler.cpp:78:73: error: missing sentinel in function call [-Werror=format=]
     char* path = nih_dbus_path(NULL, "", applicationId.constData(), NULL);
                                                                         ^

(error because of -Werror)

dobey commented 5 years ago

OK, (char *)NULL is the wrong fix for that. Those NULLs probably need to be nullptr instead. I suspect there are probably more such cases if this one is in here. Please open an issue about that.

z3ntu commented 5 years ago

Opened #23 and #24 and removed those commits from this PR.

z3ntu commented 5 years ago

@dobey (about the "missing sentinel" warning) Quote from https://stackoverflow.com/a/12122764/3527128:

The problem with the first version is that the list of parameters is meant to end with a null pointer. But '\0' is not a null pointer, it's a null character. So the value (0) is correct, it's just the type is wrong. The (char *)0 is also zero, but cast as a char pointer, which is a null pointer (ie it points to address 0)

z3ntu commented 5 years ago

I think there should be a "master" branch (for every repo) containing changes needed for the most recent versions of everything (Qt stack, etc) and then the Ubuntu-specific xenial & bionic branches can merge that stuff in if needed. I think that would be the best way to simplify support for other distros that are not Ubuntu (Touch).

dobey commented 5 years ago

I think there should be a "master" branch (for every repo) containing changes needed for the most recent versions of everything (Qt stack, etc) and then the Ubuntu-specific xenial & bionic branches can merge that stuff in if needed. I think that would be the best way to simplify support for other distros that are not Ubuntu (Touch).

I don't disagree, but that's unrelated to whether the branch would have the dependencies satisfied in CI building. Ubuntu 18.04 is a supported target for x86 builds of the Unity8 stack, and so we shouldn't merge changes which we know will break that platform. The primary development target is Ubuntu, and so for changes to merge, the changes must not break the installs on Ubuntu, regardless of whether they fix things on other distros.

dobey commented 5 years ago

I've packaged the updated qtpim for bionic. I've also opened PR #30 which merges up some changes from xenial and gets the builds passing again. As part of that, it includes the same changes as this branch, so I'm going to close this PR, since this same change is included their via the xenial changes. Thanks.