Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix requirements. JB#61688 #7

Merged
merged 3 commits into from Mar 11, 2024
Merged

Fix requirements. JB#61688 #7

merged 3 commits into from Mar 11, 2024

Conversation

pvuorela
Copy link
Contributor

@pvuorela pvuorela commented Mar 8, 2024

A bunch of garbage requirements on the .pc file. The filemanagerglobal.h is mandatory for building anything with the c++ library, so that side wasn't working at all.

@pvuorela
Copy link
Contributor Author

pvuorela commented Mar 8, 2024

Added commit for removing nemo-contacts dependency just to have one header (which shouldn't be in nemo-contacts), then doing some steps for skipping linkage to nemo-contacts and argh. In practice everyone else just copies the synchronizelists.h.

@LaakkonenJussi
Copy link

LaakkonenJussi commented Mar 8, 2024

Added commit for removing nemo-contacts dependency just to have one header (which shouldn't be in nemo-contacts), then doing some steps for skipping linkage to nemo-contacts and argh. In practice everyone else just copies the synchronizelists.h.

So, we should make that thing, the synchronizelists.h a some kind of dev-header to be included via specific package making the header being included from some common location?

@pvuorela
Copy link
Contributor Author

pvuorela commented Mar 8, 2024

So, we should make that thing, the synchronizelists.h a some kind of dev-header to be included via specific package making the header being included from some common location?

That's a possibility, but let's do it separately. Could do a task for that.

@Thaodan
Copy link

Thaodan commented Mar 8, 2024

So, we should make that thing, the synchronizelists.h a some kind of dev-header to be included via specific package making the header being included from some common location?

That's a possibility, but let's do it separately. Could do a task for that.

Why first do the hack and then the right solution? Creating a -devel package isn't witchcraft.

@LaakkonenJussi
Copy link

LaakkonenJussi commented Mar 8, 2024

So, we should make that thing, the synchronizelists.h a some kind of dev-header to be included via specific package making the header being included from some common location?

That's a possibility, but let's do it separately. Could do a task for that.

Why first do the hack and then the right solution? Creating a -devel package isn't witchcraft.

Might it be more reasonable to do that in a separate task because it would require changes to many other places than this? I mean fix the dependencies in every package that uses a copy of this synchronizelists.h? Not to mix the garbage cleanup with it as their scope is different?

@pvuorela
Copy link
Contributor Author

pvuorela commented Mar 8, 2024

Why first do the hack and then the right solution? Creating a -devel package isn't witchcraft.

Doing it properly gets first into question of introducing -devel package where? Answer is not nemo-qml-plugin-contacts as this is not really contacts specific and the file should be removed there. The same thing gets used here and there and we should not add contacts dependency to all of them. I don't even have an answer on where it should be, and my priority is now getting this library actually possible to be used.

Suspect this was just copypasted somewhere. The installed header
uses only qt things. And in addition to the unnecessary requirements
this was actually missing the proper requirement for qml (QJSValue)
It's needed by diskusage.h so the c++ library cannot be used
without it. And probably hasn't been used anywhere so far.
Requiring, linking, and then removing linkage to nemo-qml-plugin-contacts
is silly when this only needs the synchronizelist.h which shouldn't
be in contacts headers in the first place.

Just do what every other project is doing already: include own copy.
Copy link

@LaakkonenJussi LaakkonenJussi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Seems to work still on the device with the other related changes.

@Thaodan
Copy link

Thaodan commented Mar 8, 2024

Why first do the hack and then the right solution? Creating a -devel package isn't witchcraft.

Doing it properly gets first into question of introducing -devel package where? Answer is not nemo-qml-plugin-contacts as this is not really contacts specific and the file should be removed there. The same thing gets used here and there and we should not add contacts dependency to all of them. I don't even have an answer on where it should be, and my priority is now getting this library actually possible to be used.

OK makes sense.

Copy link

@Thaodan Thaodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pvuorela pvuorela merged commit 3778027 into master Mar 11, 2024
@pvuorela pvuorela deleted the fix_requirements branch March 11, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants