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

sioyek: fix typo in darwin build steps #348045

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

b-fein
Copy link

@b-fein b-fein commented Oct 12, 2024

After #283813, the postInstall step on Darwin tries to copy a file that does not exist.
Fixes the typo in the file name so that copying works.

Relevant pdf_viewer/keys_user.config file: https://github.com/ahrm/sioyek/blob/development/pdf_viewer/keys_user.config

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Oct 12, 2024
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Oct 12, 2024
@ofborg ofborg bot requested review from stephen-huan, podocarp and xyven1 October 12, 2024 12:29
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 12, 2024
Copy link
Member

@stephen-huan stephen-huan left a comment

Choose a reason for hiding this comment

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

Wow, yikes, sorry I didn't catch this in review. Looks like it happened in #283813 (comment) and none of us caught it since we don't build on macOS.

Do you happen to know why the build is failing on OfBorg? No worries if not.

Copy link
Contributor

@xyven1 xyven1 left a comment

Choose a reason for hiding this comment

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

Yeah that is really unfortunate. I still can't test on MacOS, but these changes look good to me

@stephen-huan
Copy link
Member

@xyven1 only tangentially related to this PR, but do you know what is going on in the linux postInstall?

install -Dm644 tutorial.pdf $out/share/tutorial.pdf
cp -r pdf_viewer/shaders $out/share/
install -Dm644 -t $out/etc/ pdf_viewer/{keys,prefs}.config
installManPage resources/sioyek.1

If I check with tree, keys.config and prefs.config are already copied to /etc/sioyek and shaders and tutorial.pdf are already under share/sioyek/shaders and share/sioyek/tutorial.pdf. It seems we can replace

substituteInPlace pdf_viewer/main.cpp \
--replace-fail "/usr/share/sioyek" "$out/share" \
--replace-fail "/etc/sioyek" "$out/etc"

with

     substituteInPlace pdf_viewer/main.cpp \ 
       --replace-fail "/usr/share/sioyek" "$out/share/siyoek" \ 
       --replace-fail "/etc/sioyek" "$out/etc/sioyek"

and remove lines 68--70. Having them outside clutters my nix profile (~/.local/state/nix/profiles/profile/etc) as they're in top level /etc instead of /etc/sioyek. Just a thought, maybe we shouldn't try fix what isn't broken :p.

Also we are missing keys_user.config and prefs_user.config but I guess the intention was to have those be editable in /etc (they are empty files), which is not something that terribly matters for the package.

@b-fein b-fein force-pushed the sioyek-fix-darwin-build branch 2 times, most recently from 07d25ff to 70a3675 Compare October 13, 2024 07:24
@ofborg ofborg bot requested review from stephen-huan and xyven1 October 13, 2024 08:29
@b-fein b-fein force-pushed the sioyek-fix-darwin-build branch from 70a3675 to ad80cc3 Compare October 13, 2024 12:37
@b-fein
Copy link
Author

b-fein commented Oct 13, 2024

Do you happen to know why the build is failing on OfBorg? No worries if not.

It looks like xpc should be part of the Apple SDK and should (maybe?) be included by default. Adding darwin.apple_sdk.libs.xpc explicitly as input fixes this specific build error (70a3675), but the error then is somewhere in QT), so I reverted that change again.

Copy link
Member

@stephen-huan stephen-huan left a comment

Choose a reason for hiding this comment

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

Thanks for looking into it, no worries.

@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Oct 15, 2024
@b-fein
Copy link
Author

b-fein commented Oct 23, 2024

Finished from my side, as far as I’m concerned it is ready to be merged.

@Aleksanaa
Copy link
Member

Hi, please check ofborg failure (on darwin) below

@afh
Copy link
Member

afh commented Oct 27, 2024

Result of nixpkgs-review pr 348045 run on aarch64-darwin 1

1 package failed to build:
  • sioyek

Failing with:

  In file included from /nix/store/8hlj5zsx64mfj4l3889n94wdadp9s7fm-apple-framework-IOKit-12.3/Library/Frameworks/IOKit.framework/Headers/IOKitLib.h:52:
* /nix/store/8hlj5zsx64mfj4l3889n94wdadp9s7fm-apple-framework-IOKit-12.3/Library/Frameworks/IOKit.framework/Headers/OSMessageNotification.h:120:53: error: expected ';' after top level declarator
  typedef natural_t OSAsyncReference[kOSAsyncRefCount] __kernel_ptr_semantics;

Currently at NixCon I might have time later to have a closer l👀k…

@FliegendeWurst FliegendeWurst added the awaiting_changes (old Marvin label, do not use) label Nov 8, 2024
@mathjiajia
Copy link

any update?

@booxter
Copy link
Contributor

booxter commented Nov 18, 2024

This fails for me with (after a rebase):

> /nix/store/v39c0h6xv6hvki2k1bsyp5n090q8y2bp-clang-wrapper-16.0.6/bin/clang++ -stdlib=libc++ -headerpad_max_install_names  -arch arm64 -isysroot /nix/store/amdympl4rz7kj93j82cva60v8007n4nv-apple-sdk-11.3/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk -mmacosx-version-min=11 -o sioyek.app/Contents/MacOS/sioyek  TouchSlider.o TouchCheckbox.o TouchListView.o TouchCopyOptions.o TouchRectangleSelectUI.o TouchRangeSelectUI.o TouchPageSelector.o TouchConfigMenu.o TouchTextEdit.o TouchSearchButtons.o TouchDeleteButton.o TouchHighlightButtons.o TouchSettings.o TouchAudioButtons.o TouchMarkSelector.o TouchDrawControls.o TouchMacroEditor.o TouchGenericButtons.o TouchMainMenu.o book.o config.odatabase.o document.o document_view.o input.o main.o main_widget.o pdf_renderer.o pdf_view_opengl_widget.o checksum.o new_file_checker.o coordinates.o sqlite3.o ui.o path.o utils.o mysortfilterproxymodel.o RunGuard.o OpenWithApplication.o fzf.o synctex_parser.o synctex_parser_utils.o macos_specific.o qrc_resources.o moc_TouchSlider.o moc_TouchCheckbox.o moc_TouchListView.o moc_TouchCopyOptions.o moc_TouchRectangleSelectUI.o moc_TouchRangeSelectUI.o moc_TouchPageSelector.o moc_TouchConfigMenu.o moc_TouchTextEdit.o moc_TouchSearchButtons.o moc_TouchDeleteButton.o moc_TouchHighlightButtons.o moc_TouchSettings.o moc_TouchAudioButtons.o moc_TouchMarkSelector.o moc_TouchDrawControls.o moc_TouchMacroEditor.o moc_TouchGenericButtons.o moc_TouchMainMenu.o moc_main_widget.o moc_pdf_renderer.o moc_ui.o moc_mysortfilterproxymodel.o moc_RunGuard.o moc_OpenWithApplication.o   -F/nix/store/vqdh4a4wql5g2339rfigy667cfybja79-qtdeclarative-6.8.0/lib -F/nix/store/7qps5dh35824i6a1r3zavhzmbph9zlwc-qtbase-6.8.0/lib -F/nix/store/z6czspdyq507q4kps95apxildxgsi4sp-qt3d-6.8.0/lib -F/nix/store/cgpsvgg13353wv183xmd98933p1qvc83-qtsvg-6.8.0/lib -F/nix/store/5sb3jk5jagkx0lhj0szi1xj19scmgcbr-qtspeech-6.8.0/lib -F/nix/store/nc8h7dspiyybxfd03g2iysnnr3iycc8j-qtmultimedia-6.8.0/lib -ldl -L/private/tmp/nix-build-sioyek-2.0.0-unstable-2024-09-29.drv-0/source/mupdf/build/release -lmupdf -lgumbo -lharfbuzz -lfreetype -ljbig2dec -ljpeg -lopenjp2 -lz -framework QtQuickWidgets -framework QtQuick -framework QtOpenGLWidgets -framework QtOpenGL -framework QtWidgets -framework Qt3DInput -framework Qt3DCore -framework QtSvg -framework QtTextToSpeech -l/nix/store/nc8h7dspiyybxfd03g2iysnnr3iycc8j-qtmultimedia-6.8.0/lib/QtMultimedia.framework/Versions/A/QtMultimedia -framework QtMultimedia -framework QtGui -framework AppKit -framework ImageIO -framework Metal -framework QtQmlMeta -framework QtQmlModels -framework QtQmlWorkerScript -framework QtQml -framework QtNetwork -framework QtConcurrent -framework QtCore -framework IOKit -framework DiskArbitration -framework UniformTypeIdentifiers -framework AGL -framework OpenGL
       > ld: warning: directory not found for option '-L/private/tmp/nix-build-sioyek-2.0.0-unstable-2024-09-29.drv-0/source/mupdf/build/release'
       > ld: library not found for -l/nix/store/nc8h7dspiyybxfd03g2iysnnr3iycc8j-qtmultimedia-6.8.0/lib/QtMultimedia.framework/Versions/A/QtMultimedia
       > clang-16: error: linker command failed with exit code 1 (use -v to see invocation)
       > make: *** [Makefile:544: sioyek.app/Contents/MacOS/sioyek] Error 1
       For full logs, run 'nix log /nix/store/xvqksiyrh51mba69vgqxfp7r4ixdk81w-sioyek-2.0.0-unstable-2024-09-29.drv'.
error: 1 dependencies of derivation '/nix/store/qdc2a5i9r0i7km56v5wb4g38cfjcax3l-review-shell.drv' failed to build

Though it worked before the latest SDK updates.

@khaneliman
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 348045


x86_64-darwin

❌ 1 package failed to build:
  • sioyek

aarch64-darwin

❌ 1 package failed to build:
  • sioyek

@booxter
Copy link
Contributor

booxter commented Nov 20, 2024

The immediate failure comes from the fact that the QtMultimedia framework is passed as: -l/nix/store/nc8h7dspiyybxfd03g2iysnnr3iycc8j-qtmultimedia-6.8.0/lib/QtMultimedia.framework/Versions/A/QtMultimedia -framework QtMultimedia and I think only the -framework is needed (I was able to link the file once I remove the -l argument from Makefile).

I think this -l argument comes from /nix/store/5sb3jk5jagkx0lhj0szi1xj19scmgcbr-qtspeech-6.8.0/lib/QtTextToSpeech.framework/Resources/QtTextToSpeech.prl or some other prl file:

$ cat /nix/store/5sb3jk5jagkx0lhj0szi1xj19scmgcbr-qtspeech-6.8.0/lib/QtTextToSpeech.framework/Resources/QtTextToSpeech.prl
QMAKE_PRL_TARGET = QtTextToSpeech
QMAKE_PRL_CONFIG = shared lib_bundle
QMAKE_PRL_VERSION = 6.8.0
QMAKE_PRL_LIBS = -l/nix/store/nc8h7dspiyybxfd03g2iysnnr3iycc8j-qtmultimedia-6.8.0/lib/QtMultimedia.framework/Versions/A/QtMultimedia -F/nix/store/5sb3jk5jagkx0lhj0szi1xj19scmgcbr-qtspeech-6.8.0/lib -framework QtGui -framework OpenGL -framework AGL -framework AppKit -framework ImageIO -framework Metal -framework QtNetwork -framework QtCore -framework IOKit -framework DiskArbitration -framework UniformTypeIdentifiers
QMAKE_PRL_LIBS_FOR_CMAKE = -l/nix/store/nc8h7dspiyybxfd03g2iysnnr3iycc8j-qtmultimedia-6.8.0/lib/QtMultimedia.framework/Versions/A/QtMultimedia;-F/nix/store/5sb3jk5jagkx0lhj0szi1xj19scmgcbr-qtspeech-6.8.0/lib;-framework;QtGui;-framework OpenGL;-framework AGL;-framework AppKit;-framework ImageIO;-framework Metal;-framework;QtNetwork;-framework;QtCore;-framework IOKit;-framework DiskArbitration;-framework UniformTypeIdentifiers

I have no idea why this happens though. Someone with some knowledge around qmake may have some insight...

@storvik
Copy link
Member

storvik commented Nov 20, 2024

Agree with @booxter, came to the same conclusion when investigating this.

Maybe @K900 or @emilazy knows something? Sorry for bringing you into this, but seems like you both know your way around Qt. Any help will be greatly appreciated.

@emilazy
Copy link
Member

emilazy commented Nov 20, 2024

This is probably my fault, I messed with the Darwin Qt 6 build recently. Sadly I don’t actually know that much about Qt :) but I can try and take a look. If you could try with #352395 reverted it would be helpful.

@storvik
Copy link
Member

storvik commented Nov 20, 2024

Thx for the fast reply @emilazy

Checked out b242971, the commit before the pull request you linked, and the error is still the same. So doesn't seem to be your pull request that broke this.

@emilazy
Copy link
Member

emilazy commented Nov 20, 2024

Oh good, I’m off the hook then :)

Did it build before the Qt 6.8 update? It’d be good to see the .prl file on a Nixpkgs commit from before it broke (as close to that point as possible), and from after. Alternatively, if it’s not something obvious like 6.8 or the SDK rework, we might be able to bisect the commit that caused it. I can investigate what’s going on with the .prl files, but it would be good to have a lead of some kind first.

@b-fein b-fein force-pushed the sioyek-fix-darwin-build branch from ba0f5ad to e001379 Compare December 18, 2024 04:41
@b-fein b-fein changed the title sioyek: fix build on darwin sioyek: fix typo in darwin build steps Dec 18, 2024
@b-fein
Copy link
Author

b-fein commented Dec 18, 2024

Since I have little experience with QT and definitely none whatsoever with how Nixpkgs does QT-library-stuff internally, I do not think this PR is the right place to debug this further.
I changed the commit message to reflect that only the typo is fixed. Since the overall build on Darwin is stil broken, I opened issue #366069 for further discussion regarding the QT problem.

With this: can we merge this PR?

(If I can help to test a Darwin build of sioyek in the future, feel free to ping me.)

@stephen-huan
Copy link
Member

Apologies for not being of more help, can't test on MacOS.

As a workaround for now, is it possible to revert the changes in #283813 with an overlay like the following?

final: prev:

{
  sioyek = prev.sioyek.overrideAttrs (previousAttrs: {
    src = final.fetchFromGitHub {
      owner = "ahrm";
      repo = "sioyek";
      rev = "v2.0.0";
      sha256 = "sha256-GFZaTXJhoBB+rSe7Qk6H6FZJVXr3nO9XgM+LAbS4te4=";
    };
    buildInputs = builtins.filter (pkg: final.lib.getName pkg != "qtspeech")
      previousAttrs.buildInputs or [ ];
  });
}

If I remember correctly, the changes to the build were fairly minimal. If there are problems due to the qt5 -> qt6 change, an overlay like this might work (again, untested).

final: prev:

let
  sioyek' = prev.sioyek.overrideAttrs (previousAttrs: {
    src = final.fetchFromGitHub {
      owner = "ahrm";
      repo = "sioyek";
      rev = "v2.0.0";
      sha256 = "sha256-GFZaTXJhoBB+rSe7Qk6H6FZJVXr3nO9XgM+LAbS4te4=";
    };
    buildInputs = builtins.filter (pkg: final.lib.getName pkg != "qtspeech")
      previousAttrs.buildInputs or [ ];
  });
in
{
  sioyek = final.qt5.callPackage sioyek'.override { };
}

If you're up for it, I would review a change that adds a separate Darwin derivation like what we do for signal-desktop.

With this: can we merge this PR?

Neither I nor xyven1 (I think) have merge permissions, so you can play the waiting game or ping one of the participants here who do. Or you can post on one of the NixOS discourse PR review threads like I did in #283813 (review).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 1-10 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12. first-time contribution This PR is the author's first one; please be gentle! awaiting_changes (old Marvin label, do not use)
Projects
None yet
Development

Successfully merging this pull request may close these issues.