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

libsForQt5.qtfeedback: init at unstable-2018-09-03 #121934

Merged
merged 2 commits into from May 25, 2021

Conversation

dotlambda
Copy link
Member

Motivation for this change

useful for #121345

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

I got the preConfigure line from https://github.com/OpenMandrivaAssociation/qt5-qtfeedback/blob/master/qt5-qtfeedback.spec and the postFixup one from https://git.alpinelinux.org/aports/tree/community/qt5-qtfeedback/APKBUILD.
The licenses are really confusing, but what I put is conforming to the header of e.g. https://github.com/qt/qtfeedback/blob/a14bd0bb1373cde86e09e3619fb9dc70f34c71f2/src/feedback/qfeedbackglobal.h.
I have no idea how to make the tests actually do something.
cc @NixOS/qt-kde

@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 121934 at 5e62712e run on x86_64-linux 1

3 packages built successfully:
  • libsForQt5.qtfeedback (libsForQt515.qtfeedback ,plasma5Packages.qtfeedback)
  • libsForQt512.qtfeedback
  • libsForQt514.qtfeedback

@OPNA2608
Copy link
Contributor

OPNA2608 commented May 7, 2021

useful for #121345

And #108366, I put it directly into the Qt5 package sets there (b3ee755). Dunno if it fits better there or as a separate library.

@dotlambda
Copy link
Member Author

Dunno if it fits better there or as a separate library.

I guess that's up to @NixOS/qt-kde to decide.

];

preConfigure = ''
perl ${qtbase.dev}/bin/syncqt.pl -version ${lib.getVersion qtbase}
Copy link
Contributor

Choose a reason for hiding this comment

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

Qmake can run this automatically if you leaveDotGit = true in src (I think) or manually set CONFIG+=git_build in qmakeFlags, but it's currently broken (#76080). #118904 is supposed to fix that.

The -version value is then taken from the .qmake.conf file in the root of src, which is MODULE_VERSION = 0.0.0. I don't think this version reflects the Qt version it was built against, but the module's version for API compatibility? Debian patches this to 5.0.0, Alpine leaves it as 0.0.0. 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

leaveDotGit causes the FODs to be really less reproducible.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, it was meant as more of a "this is the Qt-intended way of setting it". Should've focused more on what I was writing. 😓

.git presence sets git_build CONFIG value: https://github.com/qt/qtbase/blob/9db7cc79a26ced4997277b5c206ca15949133240/mkspecs/features/qt_build_paths.prf#L21-L22
If git_build is set, the syncqt will be prepared to automatically generate headers: https://github.com/qt/qtbase/blob/9db7cc79a26ced4997277b5c206ca15949133240/mkspecs/features/qt_module_headers.prf#L14-L15

qmakeFlags = [ "CONFIG+=git_build" ]; works better for us because that doesn't need leaveDotGit = true. I've tested this PR on top of #118904, modified to set git_build instead of calling syncqt manually, and it compiled fine. I think we should go that way instead when #118904 finds its way into master.

Copy link
Member Author

Choose a reason for hiding this comment

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

qmakeFlags = [ "CONFIG+=git_build" ]; works better for us because that doesn't need leaveDotGit = true. I've tested this PR on top of #118904, modified to set git_build instead of calling syncqt manually, and it compiled fine. I think we should go that way instead when #118904 finds its way into master.

I had tried is with git init in postPatch and that also worked on top of #118904, but your approach is better. I've rebased the PR on staging.

@dotlambda
Copy link
Member Author

Dunno if it fits better there or as a separate library.

I guess that's up to @NixOS/qt-kde to decide.

I think it should remain separate until it's officially part of Qt.

@dotlambda dotlambda changed the base branch from master to staging May 10, 2021 18:03
@dotlambda
Copy link
Member Author

I have no idea how to make the tests actually do something.

This is still bothering me.

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Diff LGTM.

@dotlambda dotlambda changed the base branch from staging to staging-next May 16, 2021 22:17
@dotlambda dotlambda changed the base branch from staging-next to master May 23, 2021 08:39
@dotlambda dotlambda merged commit 563c503 into NixOS:master May 25, 2021
@dotlambda dotlambda deleted the qtfeedback-init branch May 25, 2021 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants