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

plugins v2: port the qmake plugin #3391

Merged
merged 13 commits into from
Dec 19, 2020

Conversation

jhenstridge
Copy link
Contributor

@jhenstridge jhenstridge commented Dec 11, 2020

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh tests/unit?

This is a port of the v1 qmake plugin. It makes the following changes to the plugin properties:

  • options renamed to qmake-parameters to match other V2 plugins
  • qt-version dropped, since Ubuntu 20.04 does not ship Qt 4, and there is no Qt 6 (yet).
  • project-files dropped in favour of qmake-parameters. We're performing a srcdir=builddir build, so there is no reason to rewrite paths.

@jhenstridge jhenstridge force-pushed the plugins-v2-qmake branch 4 times, most recently from 4a76a7e to c14c81d Compare December 11, 2020 12:26
@jhenstridge jhenstridge marked this pull request as ready for review December 11, 2020 12:28
@codecov-io
Copy link

codecov-io commented Dec 11, 2020

Codecov Report

Merging #3391 (19a6144) into master (90fbe8e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3391      +/-   ##
==========================================
+ Coverage   90.76%   90.78%   +0.01%     
==========================================
  Files         255      256       +1     
  Lines       18218    18250      +32     
==========================================
+ Hits        16536    16568      +32     
  Misses       1682     1682              
Impacted Files Coverage Δ
snapcraft/plugins/_plugin_finder.py 85.00% <ø> (ø)
snapcraft/internal/pluginhandler/__init__.py 90.51% <100.00%> (+0.02%) ⬆️
snapcraft/plugins/v2/__init__.py 100.00% <100.00%> (ø)
snapcraft/plugins/v2/_plugin.py 100.00% <100.00%> (ø)
snapcraft/plugins/v2/cmake.py 95.65% <100.00%> (+0.41%) ⬆️
snapcraft/plugins/v2/meson.py 95.83% <100.00%> (+0.37%) ⬆️
snapcraft/plugins/v2/qmake.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21c4868...73261ae. Read the comment docs.

requirements.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@sergiusens sergiusens 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 contributing this. Is the goal to make use of #3392 in this PR?

def get_build_commands(self) -> List[str]:
return [
self._get_qmake_configure_command(),
'(unset CFLAGS CXXFLAGS LDFLAGS; make -j"${SNAPCRAFT_PARALLEL_BUILD_COUNT}")',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess unset is done as it is already done in _get_qmake_conflgure_commands. Mind adding a comment for that if it is the case, or if there is another reason, why the unset is done for future observers?

Copy link
Contributor

Choose a reason for hiding this comment

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

I expect that the exported ones may conflict with qmake generated ones... but certainly would appreciate a comment :D

This is certainly a fine approach. FWIW when I need to do something like this, i'll use env -u <var1>...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In particular, the generated Makefile generated by qmake for the sample snap contains:

CFLAGS        = -pipe -O2 -fPIC $(DEFINES)
CXXFLAGS      = -pipe -O2 -fPIC $(DEFINES)

Having the environment variables set during the build will override the values from the Makefile, potentially miscompiling the project.

I would not be surprised if the same is true for a few other plugins.

@jhenstridge
Copy link
Contributor Author

I think this will need some reworking if #3392 is accepted, yes. We can't fold project-files into qmake-parameters, since they would need to be rewritten to point into ${SNAPCRAFT_PART_SRC_WORK}.

cjp256
cjp256 previously approved these changes Dec 16, 2020
Copy link
Contributor

@cjp256 cjp256 left a comment

Choose a reason for hiding this comment

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

LGTM, but a comment with the unset would be icing on the cake!

Thanks!

@jhenstridge
Copy link
Contributor Author

I've rebased the plugin on top of PR #3392, and switched to out-of-tree builds. I reintroduced the qmake-project-files option, since those arguments need to be rewritten to be ${SNAPCRAFT_PART_SRC_WORK} relative.

@jhenstridge
Copy link
Contributor Author

I've changed it again to just qmake-project-file, as passing multiple project files does not seem to be supported or really make sense.

return {"g++", "make", "qt5-qmake"}

def get_build_environment(self) -> Dict[str, str]:
return {"QT_SELECT": "qt5"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Taking a note to see if this is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC, the qtchooser code is still part of the archive in 20.04, and you'd need to install qt5-default for things to function without the environment variable.

@sergiusens sergiusens merged commit 28b59f4 into canonical:master Dec 19, 2020
abitrolly pushed a commit to abitrolly/snapcraft that referenced this pull request Mar 31, 2021
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.

4 participants