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

Patch *.prl too and patch *.pc for mingw #640

Closed
wants to merge 1 commit into from

Conversation

iakov
Copy link
Contributor

@iakov iakov commented Feb 2, 2023

*.prl are qmake's library descriptions like *.pc are for package-config, and contain incorrect paths for some archives/modules (at least qttools is bad in 5.11)

Also .pc can be used with mingw via MSYS's pkgconfig, and should be patched on Windows too.

@iakov iakov changed the title Patch *.prl too and *.pc for mingw Patch *.prl too and patch *.pc for mingw Feb 2, 2023
@iakov
Copy link
Contributor Author

iakov commented Feb 2, 2023

I experienced g++: error: c:\Users\qt\work\install\lib\libQt5Widgets.a: No such file or directory
https://github.com/iakov/pythonqt/actions/runs/4075077557/jobs/7020997662#step:9:30484

@iakov iakov marked this pull request as draft February 3, 2023 07:52
@iakov iakov force-pushed the pr/fix_msvc_mingw branch 2 times, most recently from 891b73a to d47742c Compare February 3, 2023 17:17
@iakov
Copy link
Contributor Author

iakov commented Feb 3, 2023

Waiting for #641 to be merged to rebase.

@iakov iakov force-pushed the pr/fix_msvc_mingw branch 4 times, most recently from ad0cd81 to 035d1a2 Compare February 7, 2023 14:42
@iakov
Copy link
Contributor Author

iakov commented Feb 7, 2023

CI build fails with Failed to locate XML data for Qt version '5.14.2'.
Maybe a temporary problem, please, re-run a single job if there is such a possibility.

@iakov iakov marked this pull request as ready for review February 7, 2023 16:51
@ddalcino
Copy link
Contributor

I wouldn't worry about the CI failure; that's probably a server backpressure mechanism.

The only thing I can see that this PR is lacking is additional tests. I think it's clear that it's not going to break any existing functionality, but I would like to see proof in CI that this change is fixing something, and a way to prevent regressions in the future.

For now, I would not worry about unit tests; we're not testing the updater.py file thoroughly anyway. I would like to see an additional job set up in ci/generate_azure_pipelines_matrices.py that would only pass because of the changes in this PR. You would probably have to make changes to steps.yml. I'm willing to help if you need it.

What do you think, @miurahr?

@miurahr
Copy link
Owner

miurahr commented Feb 12, 2023

It is always better to have CI test that cover a code the patch changed, and integration test with actual repository.
We already have many CI tests in azure-pipelines, and increasing cases in version by version.

A CI test server, MS hosted, is provided by azure pipelines free plan.

1 Microsoft-hosted job with 1,800 minutes per month for CI/CD and 1 self-hosted job with unlimited minutes per month.

aqtinstall consumed 336 build minutes in total 360 build minutes in this month in my projects.

We can add more CI job if we can wait the jobs finished.

@ddalcino
Copy link
Contributor

A CI test server, MS hosted, is provided by azure pipelines free plan.

1 Microsoft-hosted job with 1,800 minutes per month for CI/CD and 1 self-hosted job with unlimited minutes per month.

aqtinstall consumed 336 build minutes in total 360 build minutes in this month in my projects.

Maybe it's time to audit the jobs we have and remove anything that's not really high value. I just added 6 new jobs in #648, for a total of 49 Azure builds by my count. I'm sure we can find a few that we don't need anymore.

@miurahr
Copy link
Owner

miurahr commented Feb 12, 2023

@iakov
Copy link
Contributor Author

iakov commented Feb 12, 2023

The only thing I can see that this PR is lacking is additional tests. I think it's clear that it's not going to break any existing functionality, but I would like to see proof in CI that this change is fixing something, and a way to prevent regressions in the future.
[...] I would like to see an additional job set up in ci/generate_azure_pipelines_matrices.py that would only pass because of the changes in this PR. You would probably have to make changes to steps.yml. I'm willing to help if you need it.

Ok, I like this idea to reproduce the problem with tests in CI, I hope there is even a chance to reproduce with current build configurations. In my case this happened in 5.11 win64_msvc2017_64 and with 5.11 win32_mingw73
Now as a workaround I do not install qttools module on Windows for CI builds, it helps, so there is no rush for me.

def patch_prl(self, oldvalue):
for prlfile in self.prefix.joinpath("lib").glob("*.prl"):
self.logger.info("Patching {}".format(prlfile))
self._patch_textfile(prlfile, oldvalue, "$$[QT_INSTALL_LIBS]")
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious about this value: $$[QT_INSTALL_LIBS]

I've never seen it before, but it looks like it's internal to qmake: https://doc.qt.io/qt-6/qmake-environment-reference.html

Should we be using these built-in variables to patch other values as well? It seems inconsistent to hard-code specific paths in some places, and qmake variables in others. Personally, I like the idea of using qmake variables in more places, as long as it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I treat built-in and internal use only terms differently, and propose to use built-ins instead of hard-coded paths. qmake -query shows built-ins, and for cross-platform environment these vars are usually well-defined. Worth noting, that the decision between QT_INSTALL_LIBS vs. QT_HOST_LIBS (and similar variables) should be done each time.
I can see these vars are actively used in installed prf and prl files.
Also, there are /raw, /dev , and sometimes /get properties, which are occasionaly useful. One can find examples in /mkspecs, f.e. $$[QT_INSTALL_LIBS/raw]

@miurahr
Copy link
Owner

miurahr commented Nov 12, 2023

@iakov progress?

@iakov
Copy link
Contributor Author

iakov commented Nov 14, 2023

@miurahr , sorry, I've forgotten to do this.
I've checked locally with aqtinstall==3.1.10 once again.

  1. Install Qt 5.11 with
    aqt install-qt windows desktop 5.11 win64_msvc2017_64 --archives qtwinextras qtmultimedia qtbase qttools -O c:\tmp\testQt123
  2. Now C:\tmp\testQt123\5.11.3\msvc2017_64\lib\Qt5UiTools.prl contains incorrect library reference to c:/Users/qt/work/install/lib\\Qt5Widgets.lib
  3. To test the build (linker failure) one can use empty test.cpp and dummy test.pro with the following minimalistic text
TARGET=test
QT+=uitools
SOURCES=test.cpp

then qmake to get an incorrect Makefile and nmake to get the error LINK : fatal error LNK1181: cannot open input file 'c:\Users\qt\work\install\lib\Qt5Widgets.lib'

Here are all mentioned files for convenience: test.zip

@iakov
Copy link
Contributor Author

iakov commented Nov 14, 2023

I would kindly appreciate any ideas about the CI test. Where to inject it? The example from the comment above is enough to test reproducibility, isn't it? To finish the build successfully, the test.cpp, probably, should be not empty, something like

#include <QtUiTools>
int main() { return 0; }

So, I'm ready to improve the PR, but I need an idea of the best place to add the test.

*.prl are qmake's library descriptions like *.pc are for package-config,
and contain incorrect paths for some archives/modules (at least qttools is bad in 5.11)

Also .pc can be used with mingw via MSYS's pkgconfig, and should be
patched on Windows too.
@iakov iakov force-pushed the pr/fix_msvc_mingw branch from 43ebc96 to ef20528 Compare November 14, 2023 12:30
@miurahr
Copy link
Owner

miurahr commented Nov 14, 2023

For unit test which checks a mod function of changes of Qt5UiTools.prl, for example, you can inject the test in
https://github.com/miurahr/aqtinstall/blob/master/tests/test_updater.py

Unit tests will run on GitHub Actions.

For integration test, that run qmake and built test program,
https://github.com/miurahr/aqtinstall/blob/master/ci/steps.yml
is best place.

It will be run on Azure-Pipelines.

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 this pull request may close these issues.

3 participants