Fix same bug, add cmake to ci, refactor cmake #177

KangLin · 2020-07-23T06:52:50Z

No description provided.

ftylitak · 2020-07-30T07:09:50Z

@KangLin thank you very much for your contribution. As soon as you are done with the changes, poke me for a review.

ftylitak · 2020-09-16T15:35:37Z

@KangLin thank you for the changes to the minimum version of cmake!

Since I do not personally use cmake, to complete the review I would kindly request from people that already use QZXing with cmake in their project to give it a try.

@dobey and @Milerius , would it be possible to try the proposed CMakelist from this Pull Request to your projects? Does it affect you in any way?

I really appreciate the help and effort from all of you!

Kind regards.

dobey · 2020-09-17T20:39:25Z

src/CMakeLists.txt


+option(QZXING_QML "Use qml" ${QZXING_QML})
+if(QZXING_QML)
+    target_sources(${PROJECT_NAME} PRIVATE QZXingImageProvider.cpp QZXingImageProvider.h)


I think you also need to add target_compile_definitions(${PROJECT_NAME} PRIVATE QZXING_QML) in here as well, as I'm still having to add_definitions() for it to be built.

I don't understand why you use this? You only set -DQZXING_QML=ON or -DQZXING_MULTIMEDIA=ON when run cmake configure.
When run cmake configure. It is default is ON.
You only add follow code in your CMakeLists.txt:

find_package(QZXing) add_executable(YOUR_PROJECT) target_link_libraries(${PROJECT_NAME} QZXing)

If you use qzxing source code in your project:

add_subdirectory(${QZXing_DIR}/src ${CMAKE_BINARY_DIR}/QZXing) target_link_libraries(${PROJECT_NAME} PUBLIC QZXing)

Setting only QZXING_QML=ON doesn't work because you are not setting the necessary build define to trigger the feature in the code. Thus, I still have to do add_definitions(-DQZXING_XML) in order to get the code compiled properly, otherwise there is API missing from the resulting library.

For QZXING_MULTIMEDIA it is fine, because you added the target_compile_definitions() for it in this CMakeLists.txt when it is enabled. But the target_compile_definitions() is missing for the QZXING_QML feature.

The QZXING_QML is added in follow code:

LIST(APPEND EXPORT_DEFINES QZXING_QML) # Export defines by configure_file target_compile_definitions(${PROJECT_NAME} PUBLIC ${EXPORT_DEFINES})

This is modified in the commit later. Please read all commits.

See commit 4b67917

dobey · 2020-09-17T20:47:13Z

would it be possible to try the proposed CMakelist from this Pull Request to your projects? Does it affect you in any way?

Had to make a few other changes to get things building again with the changes, and I still had to manually add a compile definition (which I mentioned in an inline comment). And I'm not sure why the default build was changed from static to shared here.

But it looks mostly OK otherwise.

KangLin · 2020-09-18T02:10:11Z

would it be possible to try the proposed CMakelist from this Pull Request to your projects? Does it affect you in any way?

Had to make a few other changes to get things building again with the changes, and I still had to manually add a compile definition (which I mentioned in an inline comment). And I'm not sure why the default build was changed from static to shared here.

But it looks mostly OK otherwise.

@dobey If you need static library, please set -DBUILD_SHARED_LIBS=OFF. Please see the README.md of changed.

dobey · 2020-09-18T17:50:58Z

@dobey If you need static library, please set -DBUILD_SHARED_LIBS=OFF. Please see the README.md of changed.

Yes, I understood this. What I meant was, is that it's not clear from this merge request why it's being changed to be an installed shared library, or why the library name was changed to the capitalized version (beyond the fact that the project(QZXing) line was not changed, I mean). There's no description of rationale for the changes in the PR description, nor any linked issue ticket to refer to and understand why the changes are being made.

KangLin · 2020-09-20T02:34:41Z

@dobey If you need static library, please set -DBUILD_SHARED_LIBS=OFF. Please see the README.md of changed.

Yes, I understood this. What I meant was, is that it's not clear from this merge request why it's being changed to be an installed shared library, or why the library name was changed to the capitalized version (beyond the fact that the project(QZXing) line was not changed, I mean). There's no description of rationale for the changes in the PR description, nor any linked issue ticket to refer to and understand why the changes are being made.

I had add dll in cmake and pro.
Modern systems generally use dynamic libraries. Because it can reduce the size of the application.

If you do not want to change, please comment the following line:

SET(BUILD_SHARED_LIBS ON CACHE BOOL "Build shared libs")

dobey · 2020-09-23T19:12:08Z

If you do not want to change, please comment the following line:

OK. This is problematic. Even when I have done set(BUILD_SHARED_LIBS OFF) in my CMakeLists.txt before including the qzxing subdirectory, it is still building qzxing as a shared lib, and is installing the library and headers with make install, which is not acceptable. Also the SO version seems wrong for the shared library build. The following lines appear when I do (cd build && make DESTDIR=/tmp/install install) in my app using qzxing:

-- Installing: /tmp/install/usr/lib/x86_64-linux-gnu/libQZXing.so.4.0.1-1-ga432920
-- Installing: /tmp/install/usr/lib/x86_64-linux-gnu/libQZXing.so
-- Installing: /tmp/install/usr/include/QZXing.h
-- Installing: /tmp/install/usr/include/QZXing_global.h
-- Installing: /tmp/install/usr/lib/x86_64-linux-gnu/cmake/QZXing/QZXingConfig.cmake
-- Installing: /tmp/install/usr/lib/x86_64-linux-gnu/cmake/QZXing/QZXingConfig-coverage.cmake
-- Installing: /tmp/install/usr/lib/x86_64-linux-gnu/cmake/QZXing/QZXingConfigVersion.cmake
-- Installing: /tmp/install/usr/lib/x86_64-linux-gnu/pkgconfig/QZXing.pc

dobey

Needs to not install headers, pkgconfig file, or shared objects (nor build them) when BUILD_SHARED_LIBS is set to off.

KangLin · 2020-09-24T00:24:41Z

If you do not want to change, please comment the following line:

OK. This is problematic. Even when I have done set(BUILD_SHARED_LIBS OFF) in my CMakeLists.txt before including the qzxing subdirectory, it is still building qzxing as a shared lib, and is installing the library and headers with make install, which is not acceptable. Also the SO version seems wrong for the shared library build. The following lines appear when I do (cd build && make DESTDIR=/tmp/install install) in my app using qzxing:
-- Installing: /tmp/install/usr/lib/x86_64-linux-gnu/libQZXing.so.4.0.1-1-ga432920
-- Installing: /tmp/install/usr/lib/x86_64-linux-gnu/libQZXing.so
-- Installing: /tmp/install/usr/include/QZXing.h
-- Installing: /tmp/install/usr/include/QZXing_global.h
-- Installing: /tmp/install/usr/lib/x86_64-linux-gnu/cmake/QZXing/QZXingConfig.cmake
-- Installing: /tmp/install/usr/lib/x86_64-linux-gnu/cmake/QZXing/QZXingConfig-coverage.cmake
-- Installing: /tmp/install/usr/lib/x86_64-linux-gnu/cmake/QZXing/QZXingConfigVersion.cmake
-- Installing: /tmp/install/usr/lib/x86_64-linux-gnu/pkgconfig/QZXing.pc

This is wrong usage. -DBUILD_SHARED_LIBS=OFF can only be specified on the command line:

cmake . -DBUILD_SHARED_LIBS=OFF

KangLin · 2020-09-24T01:22:09Z

Needs to not install headers, pkgconfig file, or shared objects (nor build them) when BUILD_SHARED_LIBS is set to off.

See a68853c

KangLin · 2020-10-02T03:37:00Z

@ftylitak Please merge it.

dobey · 2020-10-02T16:26:13Z

Sorry, I've been very busy and haven't been able to look deeper into this again, but please do not merge this yet. I don't need to be having headers and pkgconfig and shared libraries installed along with my apps when I'm explicitly telling qxzing to build static (as it was doing automatically already before).

It still also seems to be a lot of changes mixed together with no specific rationale as to why each individual part is being done, or why they all need to be done at once in a single large change.

ftylitak · 2020-10-05T10:07:35Z

@dobey thank you for your excellent review. I agree with your concerns. Within this week I will try to find the most convenient approach to not change the default behavior that the library already has while getting the benefits from @KangLin work.

Thank you both.

KangLin · 2020-10-09T01:01:44Z

Sorry, I've been very busy and haven't been able to look deeper into this again, but please do not merge this yet. I don't need to be having headers and pkgconfig and shared libraries installed along with my apps when I'm explicitly telling qxzing to build static (as it was doing automatically already before).

It still also seems to be a lot of changes mixed together with no specific rationale as to why each individual part is being done, or why they all need to be done at once in a single large change.

It is recommended that you use it as an external library, When you are use cmake.
If you must use it as embed the source code. you use cmake --build . --target install-runtime. It will not install the development library.

dobey · 2020-10-09T22:18:04Z

Sorry, I've been very busy and haven't been able to look deeper into this again, but please do not merge this yet. I don't need to be having headers and pkgconfig and shared libraries installed along with my apps when I'm explicitly telling qxzing to build static (as it was doing automatically already before).
It still also seems to be a lot of changes mixed together with no specific rationale as to why each individual part is being done, or why they all need to be done at once in a single large change.

It is recommended that you use it as an external library, When you are use cmake.
If you must use it as embed the source code. you use cmake --build . --target install-runtime. It will not install the development library.

The README talks specifically about embedding qzxing and your changes here expressly break that. Your adding of BUILD_SHARED_LIBS as a cache option overrides the internal CMake value. Removing the line you've added results in a static lib. Instead, you can pass -DBUILD_SHARED_LIBS=on when building if what you want is a shared lib. When it's not set to ON, I think no files should be installed. If I had to specify -DBUILD_SHARED_LIBS=OFF then I'm pretty sure that would break Android builds for my apps, considering for Android the app must be built as a shared object. There's also no need to change the output directories when building.

KangLin · 2020-10-10T01:27:28Z

The README talks specifically about embedding qzxing and your changes here expressly break that. Your adding of BUILD_SHARED_LIBS as a cache option overrides the internal CMake value. Removing the line you've added results in a static lib. Instead, you can pass -DBUILD_SHARED_LIBS=on when building if what you want is a shared lib. When it's not set to ON, I think no files should be installed. If I had to specify -DBUILD_SHARED_LIBS=OFF then I'm pretty sure that would break Android builds for my apps, considering for Android the app must be built as a shared object. There's also no need to change the output directories when building.

You misunderstood. BUILD_SHARED_LIBS only controls whether to generate dynamic libraries. The installation target is controlled by the INSTALL command. See: https://cmake.org/cmake/help/latest/command/install.html?highlight=install#targets

Your android program should modify the installation target and add COMPONENT. The qzxing has add COMPONENT Runtime. You can refer to the modification.

KangLin · 2020-10-21T01:10:45Z

@ftylitak Please merge it. The commit do not change the default behavior .

dobey · 2020-10-21T13:47:33Z

You misunderstood. BUILD_SHARED_LIBS only controls whether to generate dynamic libraries.

No. I did not misunderstand. Your adding it as a cache option breaks the default behavior of cmake. Setting it to OFF does not result in a static qxzing build. I tried it. I had to remove it from your changes to get a static build. You also aren't using it or any other option as a determination to install development files or not. I shoudn't need to patch qzxing to keep the same behavior that I have now when building my application with a static qzxing from git.

There is still no specified issue that this claims to fix, nor does it provide any specific rationale for the many changes. IMO it needs to at least be split up (add CI builds in one change, open issues for whatever bug(s) your'e referring to, and then maybe do refactoring if necessary separately). As it is, this is a massive set of changes which breaks current and expected behavior for developers embedding qzxing in their applications.

@KangLin

Integration of code parts from #177 provided by @KangLin to fix CI

Milerius and others added 2 commits July 23, 2020 14:24

fix(CMakeLists.txt): remove error prone extra path

114b2d1

FIX: remove error prone extra path in CMakeLists.txt

06b9940

KangLin force-pushed the master branch 15 times, most recently from 45dcb38 to 84e9a17 Compare July 24, 2020 07:52

KangLin changed the title ~~Refactor cmake~~ Fix same bug, add cmake to ci, Refactor cmake Jul 29, 2020

KangLin changed the title ~~Fix same bug, add cmake to ci, Refactor cmake~~ Fix same bug, add cmake to ci, refactor cmake Jul 29, 2020

KangLin added 7 commits July 30, 2020 14:51

Refactor cmake

f40c19d

Use iconv in windows

6a9d6c5

FIX: export dll in QZxing.pro for windows

30efb46

FIX: export dll for windows in cmake

4b67917

Modify create prl

362f4a5

Modify README.md

375c82a

Disable warning

55919aa

KangLin force-pushed the master branch from 365ec5e to 55919aa Compare July 30, 2020 06:56

KangLin added 2 commits July 30, 2020 16:41

Modify comment

890c16e

Add default CMAKE_BUILD_TYPE

5a80281

KangLin force-pushed the master branch 2 times, most recently from e22865e to 038f606 Compare September 11, 2020 04:49

dobey reviewed Sep 17, 2020

View reviewed changes

Update cmake min version

a3cb304

KangLin force-pushed the master branch from 038f606 to a3cb304 Compare September 23, 2020 02:11

dobey suggested changes Sep 23, 2020

View reviewed changes

Add install runtime

a68853c

ftylitak added a commit that referenced this pull request May 6, 2021

Integration of code parts from #177 provided by @KangLin to fix CI

3431715

ftylitak added a commit that referenced this pull request May 6, 2021

Merge pull request #197 from ftylitak/ci-testing

6b2b5a4

Integration of code parts from #177 provided by @KangLin to fix CI

dowellr85 mentioned this pull request May 7, 2021

Add CMake install target and package config #195

Closed

CMake: modify optioin variable

3cb7618

KangLin force-pushed the master branch from 44c3c09 to 3cb7618 Compare October 18, 2021 02:37

KangLin added 2 commits October 19, 2021 09:45

Private qt libraries

e175661

Change QZXING_ENABLE_ENCODER option

fb85085

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix same bug, add cmake to ci, refactor cmake #177

Fix same bug, add cmake to ci, refactor cmake #177

KangLin commented Jul 23, 2020

ftylitak commented Jul 30, 2020

ftylitak commented Sep 16, 2020

dobey Sep 17, 2020

KangLin Sep 18, 2020 •

edited

Loading

dobey Sep 18, 2020 •

edited

Loading

KangLin Sep 20, 2020 •

edited

Loading

KangLin Sep 20, 2020 •

edited

Loading

dobey commented Sep 17, 2020

KangLin commented Sep 18, 2020 •

edited

Loading

dobey commented Sep 18, 2020

KangLin commented Sep 20, 2020 •

edited

Loading

dobey commented Sep 23, 2020

dobey left a comment

KangLin commented Sep 24, 2020

KangLin commented Sep 24, 2020

KangLin commented Oct 2, 2020

dobey commented Oct 2, 2020

ftylitak commented Oct 5, 2020

KangLin commented Oct 9, 2020

dobey commented Oct 9, 2020

KangLin commented Oct 10, 2020

KangLin commented Oct 21, 2020 •

edited

Loading

dobey commented Oct 21, 2020

Fix same bug, add cmake to ci, refactor cmake #177

Are you sure you want to change the base?

Fix same bug, add cmake to ci, refactor cmake #177

Conversation

KangLin commented Jul 23, 2020

ftylitak commented Jul 30, 2020

ftylitak commented Sep 16, 2020

dobey Sep 17, 2020

Choose a reason for hiding this comment

KangLin Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

dobey Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

KangLin Sep 20, 2020 • edited Loading

Choose a reason for hiding this comment

KangLin Sep 20, 2020 • edited Loading

Choose a reason for hiding this comment

dobey commented Sep 17, 2020

KangLin commented Sep 18, 2020 • edited Loading

dobey commented Sep 18, 2020

KangLin commented Sep 20, 2020 • edited Loading

dobey commented Sep 23, 2020

dobey left a comment

Choose a reason for hiding this comment

KangLin commented Sep 24, 2020

KangLin commented Sep 24, 2020

KangLin commented Oct 2, 2020

dobey commented Oct 2, 2020

ftylitak commented Oct 5, 2020

KangLin commented Oct 9, 2020

dobey commented Oct 9, 2020

KangLin commented Oct 10, 2020

KangLin commented Oct 21, 2020 • edited Loading

dobey commented Oct 21, 2020

KangLin Sep 18, 2020 •

edited

Loading

dobey Sep 18, 2020 •

edited

Loading

KangLin Sep 20, 2020 •

edited

Loading

KangLin Sep 20, 2020 •

edited

Loading

KangLin commented Sep 18, 2020 •

edited

Loading

KangLin commented Sep 20, 2020 •

edited

Loading

KangLin commented Oct 21, 2020 •

edited

Loading