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

qpdf: split outputs; modernize #326390

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

getchoo
Copy link
Member

@getchoo getchoo commented Jul 11, 2024

Description of changes

Came across lib.getExe not working with this package in #326372

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.

@getchoo getchoo mentioned this pull request Jul 11, 2024
13 tasks
@ofborg ofborg bot requested a review from abbradar July 11, 2024 22:37
@getchoo getchoo marked this pull request as draft July 11, 2024 22:48
@getchoo getchoo force-pushed the pkgs/qpdf/mainProgram branch from 5f2c5ea to 5bed75a Compare July 11, 2024 22:48
@getchoo getchoo changed the base branch from master to staging July 11, 2024 22:48
@getchoo getchoo force-pushed the pkgs/qpdf/mainProgram branch from 5bed75a to 3a571c3 Compare July 11, 2024 22:50
@getchoo getchoo marked this pull request as ready for review July 11, 2024 22:50
@getchoo
Copy link
Member Author

getchoo commented Jul 11, 2024

rebased onto staging as this will cause mass rebuilds

@doronbehar
Copy link
Contributor

@ofborg eval

@getchoo getchoo force-pushed the pkgs/qpdf/mainProgram branch from 3a571c3 to 3beee00 Compare July 12, 2024 07:43
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.

Looks good overall!

pkgs/development/libraries/qpdf/default.nix Outdated Show resolved Hide resolved
@getchoo getchoo force-pushed the pkgs/qpdf/mainProgram branch from 3beee00 to c801f38 Compare July 14, 2024 19:48
@getchoo getchoo requested a review from doronbehar July 14, 2024 19:49
@getchoo getchoo force-pushed the pkgs/qpdf/mainProgram branch from c801f38 to e3a3091 Compare July 14, 2024 22:04
@getchoo getchoo requested a review from doronbehar July 14, 2024 22:06
@doronbehar
Copy link
Contributor

Splitting outputs sometimes causes breakages in dependent packages... I tested the build locally, (against nixos-unstable), and the contents in the different outputs seems almost perfect to me. I'll list it here just for reference:

$ tree $(nix build --print-out-paths --no-link -f. qpdf.{bin,dev,doc,lib,man,out}
/nix/store/3c9a59g8gcfpjsibp4lcx0r6rw7xkvq6-qpdf-11.9.1-bin
└── bin
    ├── fix-qdf
    ├── qpdf
    └── zlib-flate
/nix/store/vbyg26aw7xshfdhwn1h3c6v6j1hhzhyk-qpdf-11.9.1-dev
├── include
│   └── qpdf
│       ├── auto_job_c_att.hh
│       ├── auto_job_c_copy_att.hh
│       ├── auto_job_c_enc.hh
│       ├── auto_job_c_main.hh
│       ├── auto_job_c_pages.hh
│       ├── auto_job_c_set_page_labels.hh
│       ├── auto_job_c_uo.hh
│       ├── Buffer.hh
│       ├── BufferInputSource.hh
│       ├── ClosedFileInputSource.hh
│       ├── Constants.h
│       ├── DLL.h
│       ├── FileInputSource.hh
│       ├── InputSource.hh
│       ├── JSON.hh
│       ├── PDFVersion.hh
│       ├── Pipeline.hh
│       ├── Pl_Buffer.hh
│       ├── Pl_Concatenate.hh
│       ├── Pl_Count.hh
│       ├── Pl_DCT.hh
│       ├── Pl_Discard.hh
│       ├── Pl_Flate.hh
│       ├── Pl_Function.hh
│       ├── Pl_OStream.hh
│       ├── Pl_QPDFTokenizer.hh
│       ├── Pl_RunLength.hh
│       ├── Pl_StdioFile.hh
│       ├── Pl_String.hh
│       ├── PointerHolder.hh
│       ├── QIntC.hh
│       ├── QPDFAcroFormDocumentHelper.hh
│       ├── QPDFAnnotationObjectHelper.hh
│       ├── qpdf-c.h
│       ├── QPDFCryptoImpl.hh
│       ├── QPDFCryptoProvider.hh
│       ├── QPDFDocumentHelper.hh
│       ├── QPDFEFStreamObjectHelper.hh
│       ├── QPDFEmbeddedFileDocumentHelper.hh
│       ├── QPDFExc.hh
│       ├── QPDFFileSpecObjectHelper.hh
│       ├── QPDFFormFieldObjectHelper.hh
│       ├── QPDF.hh
│       ├── qpdfjob-c.h
│       ├── QPDFJob.hh
│       ├── qpdflogger-c.h
│       ├── QPDFLogger.hh
│       ├── QPDFMatrix.hh
│       ├── QPDFNameTreeObjectHelper.hh
│       ├── QPDFNumberTreeObjectHelper.hh
│       ├── QPDFObjectHandle.hh
│       ├── QPDFObjectHelper.hh
│       ├── QPDFObject.hh
│       ├── QPDFObjGen.hh
│       ├── QPDFOutlineDocumentHelper.hh
│       ├── QPDFOutlineObjectHelper.hh
│       ├── QPDFPageDocumentHelper.hh
│       ├── QPDFPageLabelDocumentHelper.hh
│       ├── QPDFPageObjectHelper.hh
│       ├── QPDFStreamFilter.hh
│       ├── QPDFSystemError.hh
│       ├── QPDFTokenizer.hh
│       ├── QPDFUsage.hh
│       ├── QPDFWriter.hh
│       ├── QPDFXRefEntry.hh
│       ├── QTC.hh
│       ├── QUtil.hh
│       ├── RandomDataProvider.hh
│       └── Types.h
├── lib
│   ├── cmake
│   │   └── qpdf
│   │       ├── libqpdfTargets.cmake
│   │       ├── libqpdfTargets-release.cmake
│   │       ├── qpdfConfig.cmake
│   │       └── qpdfConfigVersion.cmake
│   └── pkgconfig
│       └── libqpdf.pc
└── nix-support
    └── propagated-build-inputs
/nix/store/4zhsaqmd06s4fcgmpdjg7a23xhx15j2j-qpdf-11.9.1-doc
└── share
    └── doc
        └── qpdf
            ├── examples
            │   ├── extend-c-api.c
            │   ├── extend-c-api.h
            │   ├── extend-c-api-impl.cc
            │   ├── pdf-attach-file.cc
            │   ├── pdf-bookmarks.cc
            │   ├── pdf-c-objects.c
            │   ├── pdf-count-strings.cc
            │   ├── pdf-create.cc
            │   ├── pdf-custom-filter.cc
            │   ├── pdf-double-page-size.cc
            │   ├── pdf-filter-tokens.cc
            │   ├── pdf-invert-images.cc
            │   ├── pdf-linearize.c
            │   ├── pdf-mod-info.cc
            │   ├── pdf-name-number-tree.cc
            │   ├── pdf-npages.cc
            │   ├── pdf-overlay-page.cc
            │   ├── pdf-parse-content.cc
            │   ├── pdf-set-form-values.cc
            │   ├── pdf-split-pages.cc
            │   ├── qpdf-job.cc
            │   ├── qpdfjob-c.c
            │   ├── qpdfjob-c-save-attachment.c
            │   ├── qpdfjob-remove-annotations.cc
            │   └── qpdfjob-save-attachment.cc
            └── README-doc.txt
/nix/store/yzn4h9lfrscfkqjmzs6aw35g7qr2lw3g-qpdf-11.9.1-lib
└── lib
    ├── libqpdf.a
    ├── libqpdf.so -> libqpdf.so.29
    ├── libqpdf.so.29 -> libqpdf.so.29.9.1
    └── libqpdf.so.29.9.1
/nix/store/1h14r7rfxgr2ihgwani0zsq66zdsrqfn-qpdf-11.9.1-man
└── share
    └── man
        └── man1
            ├── fix-qdf.1.gz
            ├── qpdf.1.gz
            └── zlib-flate.1.gz
/nix/store/rcz5hfbl23hg02330dwff1isbw6haicf-qpdf-11.9.1

21 directories, 111 files

The only problem I found, was the contents of ${qpdf.dev}/lib/cmake/qpdf/libqpdfTargets.cmake:

# The installation prefix configured by this project.
set(_IMPORT_PREFIX "/nix/store/rcz5hfbl23hg02330dwff1isbw6haicf-qpdf-11.9.1")

# Create imported target qpdf::libqpdf
add_library(qpdf::libqpdf SHARED IMPORTED)

set_target_properties(qpdf::libqpdf PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
)

# Create imported target qpdf::libqpdf_static
add_library(qpdf::libqpdf_static STATIC IMPORTED)

set_target_properties(qpdf::libqpdf_static PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "/nix/store/rc18a8k50zmrif61250sfidkqvlg41ln-zlib-1.3.1/lib/libz.so;/nix/store/0dsiifpjc1cxbaa8d6b0kc6d2wijk7in-libjpeg-turbo-3.0.3/lib/libjpeg.so"
)

Note how the out output is an empty directory, and the _IMPORT_PREFIX is pointing to it... Perhaps the .dev output should be removed, and then the out output will include the cmake, pkgconfig files etc, and the cmake files will use the correct prefix.

@getchoo getchoo force-pushed the pkgs/qpdf/mainProgram branch from e3a3091 to f9330b6 Compare July 15, 2024 20:15
@getchoo
Copy link
Member Author

getchoo commented Jul 15, 2024

Note how the out output is an empty directory, and the _IMPORT_PREFIX is pointing to it... Perhaps the .dev output should be removed, and then the out output will include the cmake, pkgconfig files etc, and the cmake files will use the correct prefix.

Thanks for catching this! I've built a few packages with this change as well and all seems good :)

@doronbehar
Copy link
Contributor

Note how the out output is an empty directory, and the _IMPORT_PREFIX is pointing to it... Perhaps the .dev output should be removed, and then the out output will include the cmake, pkgconfig files etc, and the cmake files will use the correct prefix.

Thanks for catching this! I've built a few packages with this change as well and all seems good :)

I imagine that the packages you tested were using the pkgconfig file, and not the cmake API. However, inspecting in a similar manner the outputs' contents shows promising prospect.

@doronbehar
Copy link
Contributor

I will merge once ofborg evaluation is finished.

@doronbehar doronbehar merged commit df8b62e into NixOS:staging Jul 16, 2024
24 of 26 checks passed
@getchoo getchoo deleted the pkgs/qpdf/mainProgram branch July 21, 2024 07:45
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.

2 participants