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

bug in DeveloperTips wiki page #1274

Open
Germano0 opened this issue Jun 20, 2024 · 19 comments · May be fixed by #1277
Open

bug in DeveloperTips wiki page #1274

Germano0 opened this issue Jun 20, 2024 · 19 comments · May be fixed by #1277

Comments

@Germano0
Copy link

Hello, Fedora package co-maintainer here.
There is a bug in the DeveloperTips wiki page: the section

Replace line  
https://github.com/open-eid/qt-common/blob/master/CMakeLists.txt#L57   
`qt_add_resources(CONFIG_SOURCES ${CMAKE_CURRENT_BINARY_DIR}/config.qrc)`   
with   
`qt_add_resources(CONFIG_SOURCES config.qrc)` 

contains a link that does not point to the

`qt_add_resources(CONFIG_SOURCES ${CMAKE_CURRENT_BINARY_DIR}/config.qrc)`   

line. Instead it points to

target_compile_definitions(qdigidoccommon PUBLIC CONFIG_URL="${CONFIG_URL}")

line.
Also, the current approach to CMake usage is quite prone to errors for maintainers, and the Fedora developer @LecrisUT in an internal discussion proposed the usage of https://cmake.org/cmake/help/v3.30/module/FetchContent.html
He will soon write a comment explaining his view.
Best regards

@metsma
Copy link
Contributor

metsma commented Jun 20, 2024

Thats why there is code reference in wiki when code changes you can still find relevant parts.
How is the fetchcontent better current download solution?

@LecrisUT
Copy link

Hi,

FetchContent is the recommended workflow for modern CMake because it allows the maximum flexibility for users, developers, packagers and downstream consumer. While it is designed for integrating with CMake dependencies it can and should be used for downloading arbitrary files. Some features:

  • it allows using (offline) lookaside sources which is what we would need for Fedora packaging
  • de-duplicates downloading in add_subdirectory or FetchContent chains
  • efficient caching that can work across build environments (if configured)
  • allows downstream to override settings, e.g. you don't need to expose options for download url

Please use this especially for replacing find_package, because there is native support to have fallback from find_package to FetchContent, with built-in options to preffer the latter or only use the former.


Regarding theacomment in the DevelopersTip, I wish that you don't find it offensive, but I agree that such workflow is inappropriate, because for starters the links are pointing to a master blob which will change over time, and the CMake recipe should be well-defined by the project authors to express what is supported and not through usage of options.

There is of course multiple alternative designs besides FetchContent to avoid the workflow in the DevelopersTip, simplest being to control it with options, but because we are not developers of the project, I at least cannot give more detailed suggestions or PR support without a bit of context, knowledge of requirements, or forbidden setups from the project developers. I would happily offer review and advice support on anything CMake related.

@metsma
Copy link
Contributor

metsma commented Jun 20, 2024

My concern is that it should not be offline. The config files should be updated regularly. Least once on every release.
https://src.fedoraproject.org/rpms/qdigidoc/blob/rawhide/f/config.json
As example 20230504110015Z over year old config is packaged. How can we avoid this?
Does the FetchContent solve this somehow? Does it still download the resources online somehow?

@LecrisUT
Copy link

Thanks for the explanation. Would it make sense to use a versioning like 4.6.0^YYYYMMDD? On the Fedora packaging side unfortunately all sources must be vetted for licensing issues and uploaded manually (or semi-manually).

FetchContent by default will download the files, so your primary build will not be affected, but on the Fedora side we will have to use FETCHCONTENT_SOURCE_DIR_<uppercaseName> to redirect where this should point to because we do not have internet connection when doing a build.

We can however offer upstream more control of this process to perform this update more regularly. But it depends how regular is this required?

The config files should be updated regularly. Least once on every release.

This at least we can accommodate without any infrastructure change (except for the handling the download section in the code). @Germano0 can you make sure there is a note in the README downstream to remind maintainers to update this file on each release?


However I think having the build dependent on something that changes so regularly is a design flaw. I would recommend three possible approaches (not mutually exclusive):

  • Making the download of the file at runtime or through an utility. There are numerous libraries that can help implement an https connection like boost.beast
  • Separate the build part/library that uses the downloaded source to a more minimal component so that it can be run individually more frequently
  • Simply read the file from a pre-defined location in ${DATAROOTDIR}. A simple curl script to download the most recent file can be provided

@metsma
Copy link
Contributor

metsma commented Jun 21, 2024

On the Fedora packaging side unfortunately all sources must be vetted for licensing issues and uploaded manually

I am actually not sure how the config and TSL-s are licensed.

However I think having the build dependent on something that changes so regularly is a design flaw.

This is something that EU designed to distribute digital signature trust chains and there are many moving parts.
It is updated by DigiDoc regularly but packaging gives good first offline impression when validating files.
https://eidas.ec.europa.eu/efda/tl-browser/#/screen/home

There is already tool for downloading TSL-s
https://github.com/open-eid/DigiDoc4-Client/blob/master/client/TSLDownload.cpp

I can probably change the build if files are already in build directory or least in source directory then it will skip the downloading part and continue.

@LecrisUT
Copy link

I am actually not sure how the config and TSL-s are licensed.

This would depend on the distribution page, but I don't see anything obvious in the header metadata, and top page (https://id.eesti.ee/) is empty. Ianal, but these could be simply unlicensed. It would be good if someone can confirm these.

I can probably change the build if files are already in build directory or least in source directory then it will skip the downloading part and continue.

Yes, of course that would work as well. FetchContent is more modern and native supported approach for this, which can minimize maintenance, since most features like caching are already baked in.

This is something that EU designed to distribute digital signature trust chains and there are many moving parts.
It is updated by DigiDoc regularly but packaging gives good first offline impression when validating files.
https://eidas.ec.europa.eu/efda/tl-browser/#/screen/home

Thanks for the reference, I will forward this to see how we can incorporate it with our monitoring backend (https://release-monitoring.org)


What would be very helpful, for designing the CMake project, managing the monitor and for packaging is if all of these files can be aggregated into a single folder (or better yet, and archive). Having a script to check the downloaded/local sources daily and update if contents change should be rather trivial to implement, if not already provided as a service somewhere. I could ask the forum for their experience on such tools if it helps.

@LecrisUT
Copy link

Another point that will help is to understand the distinction between the various downloaded files:

URL:            https://github.com/open-eid/DigiDoc4-Client
Source0:        %{url}/releases/download/v%{version}/%{upstream_name}-%{version}.tar.gz
# https://id.eesti.ee/config.json
Source1:        config.json
# https://id.eesti.ee/config.pub
Source2:        config.pub
# https://id.eesti.ee/config.rsa
Source3:        config.rsa
# https://github.com/open-eid/DigiDoc4-Client/wiki/DeveloperTips
Source4:        config.qrc
# https://sr.riik.ee/tsl/estonian-tsl.xml
Source5:        EE.xml
# https://github.com/open-eid/DigiDoc4-Client/wiki/DeveloperTips
Source6:        TSL.qrc
# https://ec.europa.eu/tools/lotl/eu-lotl.xml`
Source7:        eu-lotl.xml

For example, config.json seems rather meaningless to have downloaded. It could be included in the github repo and have the server redirect to the raw file url.

  • Which sources are necessary to update regularly, my guess is only eu-lotl.xml?
  • Which sources are used only at build time
  • If we are to track a file, which one should it be? Is there a way to get a date of when that file is updated? Probably this can work with a plain ftp source.

@metsma
Copy link
Contributor

metsma commented Jun 21, 2024

For example, config.json seems rather meaningless to have downloaded. It could be included in the github repo and have the server redirect to the raw file url.

What do you mean?

Only file config.pub that does not change is public key file.
config.json, config.rsa change regularly.
Cert pinning certificates/version change notification/possible to disable out of support digidoc versions and etc.
The TSL files expire in every 6 months + additional changes.

@LecrisUT
Copy link

Sorry, I meant config.qrc

@metsma
Copy link
Contributor

metsma commented Jun 21, 2024

Sorry, I meant config.qrc

It is already in repo
https://github.com/open-eid/qt-common/blob/master/config.qrc

@LecrisUT
Copy link

Sorry, I meant config.qrc

It is already in repo https://github.com/open-eid/qt-common/blob/master/config.qrc

Ok, must have been outdated from 4.4.0.

Then what about the remaining points?

  • Which sources are necessary to update regularly, my guess is only eu-lotl.xml?

  • Which sources are used only at build time

  • If we are to track a file, which one should it be? Is there a way to get a date of when that file is updated? Probably this can work with a plain ftp source.

A more specific breakdown for the files. The idea is that if we make the versioning like 4.6.0^YYYYMMDD to incorporate the date of the certificate updates, which ones are necessary for the user, which ones are necessary only at compile time, and which ones are "must be updated" in order for the program to function. That would help steer how we can add these to release-monitoring.org, and figure out if it's possible to separate the main app binary from the certificates.

@metsma
Copy link
Contributor

metsma commented Jun 21, 2024

I think updating files with DigiDoc release is good enough.
config.json, config.rsa, eu-lotl.xml, EE.xml.
All the files are also updated regularly on runtime.

@LecrisUT
Copy link

All the files are also updated regularly on runtime.

Aaah, perfect to know. I was just over-complicating things. Than we can just add a note to update those files whenever a new release is published. I was afraid that if we were using too old of files, it could brick itself.

Then the only thing that we would need are possibilities to redirect the source of these files. Feel free to implement whatever approach works best for you, and if you need eyes to review, feel free to ping me. The minimum that we would need is to remove the need for manual patching, and to be able to point to the pre-downloaded files.

Here are some snippets that you could use for reference:

  • FetchContent approach
    # file( DOWNLOAD ${CONFIG_URL} ${CMAKE_CURRENT_BINARY_DIR}/config.json)
    FetchContent_Declare(config_json
        URL ${CONFIG_URL}
    )
    FetchContent_MakeAvailable(config_json)
    configure_file(
        ${config_json_SOURCE_DIR}/config.json
        ${CMAKE_CURRENT_BINARY_DIR}/config.json
        COPYONLY
    )
  • Simple file checker
    # file( DOWNLOAD ${CONFIG_URL} ${CMAKE_CURRENT_BINARY_DIR}/config.json)
    # One of the rare occasions where CMAKE_SOURCE_DIR is justified
    if (EXISTS ${CMAKE_SOURCE_DIR}/config.json)
        configure_file(
            ${CMAKE_SOURCE_DIR}/config.json
            ${CMAKE_CURRENT_BINARY_DIR}/config.json
            COPYONLY
        )
    else()
        file( DOWNLOAD ${CONFIG_URL} ${CMAKE_CURRENT_BINARY_DIR}/config.json)
    endif()

Navigating a bit I find that these should be updated upstream at open-eid/qt-common. That should be a standalone (importable) CMake project that is used via FetchContent/find_package, but I degrees.

Regarding add_custom_command(OUTPUT TSL.qrc), it can work with a similar if (EXISTS) guard, but we can also write the file in the build directory before running the cmake --build (assuming TSL.qrc, EE.xml, and eu-lotl.xml are the only artifacts created there). It would be rather problematic if any of the filenames/sturcutre change.

@pxrb
Copy link

pxrb commented Jun 22, 2024

Hello, i would like to say that i am having the same problem. I am trying to package digidoc4-client for gentoo, I got most of the fixing done, its just that one particular command:

/usr/bin/x86_64-pc-linux-gnu-g++ -DBUILD_DATE=\"22.06.2024\" -DBUILD_VER=0 -DCDOC2_GET_URL=\"https://cdoc2-keyserver-get\" -DCDOC2_POST_URL=\"https://cdoc2-keyserver-post\" -DCONFIG_URL=\"https://id.eesti.ee/config.json\" -DMAJOR_VER=4 -DMINOR_VER=5 -DMOBILEID_URL=\"https://dd-mid.ria.ee/mid-api\" -DQT_CORE_LIB -DQT_DEPRECATED_WARNINGS_SINCE=051200 -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_NO_DEBUG -DQT_PRINTSUPPORT_LIB -DQT_SVG_LIB -DQT_WIDGETS_LIB -DRELEASE_VER=1 -DSMARTID_URL=\"https://dd-sid.ria.ee/v1\" -I/var/tmp/portage/app-crypt/digidoc4-client-4.5.1-r1/work/qdigidoc4-4.5.1_build/client -I/var/tmp/portage/app-crypt/digidoc4-client-4.5.1-r1/work/qdigidoc4-4.5.1/client -I/var/tmp/portage/app-crypt/digidoc4-client-4.5.1-r1/work/qdigidoc4-4.5.1_build/client/qdigidoc4_autogen/include -I/var/tmp/portage/app-crypt/digidoc4-client-4.5.1-r1/work/qdigidoc4-4.5.1 -I/var/tmp/portage/app-crypt/digidoc4-client-4.5.1-r1/work/qdigidoc4-4.5.1/common -isystem /usr/include/qt5 -isystem /usr/include/qt5/QtNetwork -isystem /usr/include/qt5/QtCore -isystem /usr/lib64/qt5/mkspecs/linux-g++ -isystem /usr/include/qt5/QtWidgets -isystem /usr/include/qt5/QtGui -isystem /usr/include/qt5/QtPrintSupport -isystem /usr/include/qt5/QtSvg  -march=native -O2 -pipe -std=gnu++17 -flto=auto -fno-fat-lto-objects -fvisibility=hidden -fvisibility-inlines-hidden -fPIC -MD -MT client/CMakeFiles/qdigidoc4.dir/qdigidoc4_autogen/4UGIYD6VGQ/qrc_images.cpp.o -MF client/CMakeFiles/qdigidoc4.dir/qdigidoc4_autogen/4UGIYD6VGQ/qrc_images.cpp.o.d -o client/CMakeFiles/qdigidoc4.dir/qdigidoc4_autogen/4UGIYD6VGQ/qrc_images.cpp.o -c /var/tmp/portage/app-crypt/digidoc4-client-4.5.1-r1/work/qdigidoc4-4.5.1_build/client/qdigidoc4_autogen/4UGIYD6VGQ/qrc_images.cpp

is failing, i assume that this is because it is trying to connect to the internet and since digidoc4-client has been updated alot, and the wiki page for Sandboxed environments isn't updated, it doesnt include instructions on how to fix this.

@Germano0
Copy link
Author

@pxrb for a better code impagination, use
```
code
```

4.5.1 works because yesterday I managed to build it
https://src.fedoraproject.org/rpms/qdigidoc/tree/rawhide
Also make sure you have `1251.patch'

@pxrb
Copy link

pxrb commented Jun 22, 2024

Thanks. It finally compiled.

@metsma
Copy link
Contributor

metsma commented Jun 30, 2024

@pxrb for a better code impagination, use code

4.5.1 works because yesterday I managed to build it https://src.fedoraproject.org/rpms/qdigidoc/tree/rawhide Also make sure you have `1251.patch'

Please use Qt6

metsma added a commit to metsma/qt-common that referenced this issue Jun 30, 2024
metsma added a commit to metsma/qt-common that referenced this issue Jun 30, 2024
metsma added a commit to metsma/qt-common that referenced this issue Jul 1, 2024
metsma added a commit to metsma/DigiDoc4-Client that referenced this issue Jul 1, 2024
IB-8136, Fixes open-eid#1274

Signed-off-by: Raul Metsma <raul@metsma.ee>
@metsma
Copy link
Contributor

metsma commented Jul 1, 2024

Does #1277 resolve this issue?

@LecrisUT
Copy link

LecrisUT commented Jul 1, 2024

Indeed, that's an acceptable minimal solution 👍. But don't forget to update the wiki page to reflect the difference, and where the pre-downloaded resource files should point to (/ or /common)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants