Skip to content

Repaired pythonModules.qscintilla-qt5#71641

Merged
jonringer merged 1 commit intoNixOS:masterfrom
bene1618:qscintilla-qt5
Oct 28, 2019
Merged

Repaired pythonModules.qscintilla-qt5#71641
jonringer merged 1 commit intoNixOS:masterfrom
bene1618:qscintilla-qt5

Conversation

@bene1618
Copy link
Contributor

@bene1618 bene1618 commented Oct 22, 2019 •
edited
Loading

Motivation for this change

It seems that qmake does not automatically make QtWidgets header files visible. My change patches the configure.py file which in turn generates the Project file used with qmake, so that the relevant include files become visible at build time, and the package is no longer broken.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • [ x ] NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @lsix

jonringer
jonringer reviewed Oct 22, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

please rename your commit pythonPackages.qscintilla-qt5: fix build

@ofborg ofborg bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Oct 22, 2019
@ofborg ofborg bot requested a review from lsix October 22, 2019 09:00
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Oct 22, 2019
lsix
lsix suggested changes Oct 23, 2019
View reviewed changes
Copy link
Member

@lsix lsix left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks for the PR.

I have very limited internet connectivity this week, so I cannot properly test your PR (if no-one has done it then, I’ll surely do it next week).

I have attached a few comments of what I can see before testing.

@ofborg ofborg bot requested a review from lsix October 23, 2019 12:56
@lsix lsix mentioned this pull request Oct 23, 2019
Merged
10 tasks
@das-g
Copy link
Member

das-g commented Oct 24, 2019

  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"

I've run LC_ALL=C.utf-8 nix run nixpkgs.nix-review -c nix-review pr 71641 and got this output:

$ git fetch --force https://github.com/NixOS/nixpkgs master:refs/nix-review/0 pull/71641/head:refs/nix-review/1
$ git worktree add /home/das-g/.cache/nix-review/pr-71641-2/nixpkgs c6af1f6e03c1e82e7c7d30cc955d3387b685150d
Preparing worktree (detached HEAD c6af1f6e03c)
HEAD is now at c6af1f6e03c verilator: 4.018 -> 4.020
$ git merge --no-commit ad2bc2ead0e1c34769d56a9484851e82ec9ece41
Automatic merge went well; stopped before committing as requested
$ nix build --no-link --keep-going --max-jobs 8 --option build-use-sandbox true -f /home/das-g/.cache/nix-review/pr-71641-2/build.nix
warning: ignoring the user-specified setting 'sandbox', because it is a restricted setting and you are not a trusted user
https://github.com/NixOS/nixpkgs/pull/71641
5 package were build:
python27Packages.qscintilla-qt5 python37Packages.qscintilla-qt5 python38Packages.qscintilla-qt5 qgis qgis-unwrapped

$ nix-shell /home/das-g/.cache/nix-review/pr-71641-2/shell.nix
  • In the resulting nix-shell, I was able to start QGIS by executing qgis. 👍

das-g
das-g reviewed Oct 24, 2019
View reviewed changes
Copy link
Member

@das-g das-g left a comment

Choose a reason for hiding this comment

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

The change itself looks good to me. The two commits should probably be squashed into a single one with commit message pythonPackages.qscintilla-qt5: fix build. (Especially since ad2bc2e not only replaces sed by substituteInPlace, but also makes the fix actually take effect.)

@lsix
Copy link
Member

lsix commented Oct 24, 2019

@bene1618, as suggested by @das-g, can you squash the commits ?

@jonringer
Copy link
Contributor

jonringer commented Oct 24, 2019

if you need help with your git-fu

to fold latest commit into previous:

git reset HEAD^
git commit -a --amend --no-edit

jonringer
jonringer approved these changes Oct 24, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM
able to open qgis

[6 built, 390 copied (2044.5 MiB), 374.5 MiB DL]
https://github.com/NixOS/nixpkgs/pull/71641
5 package were build:
python27Packages.qscintilla-qt5 python37Packages.qscintilla-qt5 python38Packages.qscintilla-qt5 qgis qgis-unwrapped

just waiting on squash

@bene1618
Copy link
Contributor Author

bene1618 commented Oct 26, 2019 

git reset HEAD^
git commit -a --amend --no-edit

Thank you, I squashed the two commits.

@jonringer
Copy link
Contributor

jonringer commented Oct 26, 2019

@GrahamcOfBorg build python27Packages.qscintilla-qt5 python37Packages.qscintilla-qt5 python38Packages.qscintilla-qt5

jonringer
jonringer approved these changes Oct 26, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM
able to start qgis fine

[6 built, 15 copied (672.7 MiB), 87.3 MiB DL]
https://github.com/NixOS/nixpkgs/pull/71641
5 package were build:
python27Packages.qscintilla-qt5 python37Packages.qscintilla-qt5 python38Packages.qscintilla-qt5 qgis qgis-unwrapped

@jonringer
Copy link
Contributor

jonringer commented Oct 26, 2019

looks like there's quite a bit of darwin failures in the fixup phase @NixOS/darwin-maintainers ? I think the patching of shebangs is causing it to fail

das-g
das-g approved these changes Oct 27, 2019
View reviewed changes
Copy link
Member

@das-g das-g left a comment

Choose a reason for hiding this comment

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

👍

@das-g
Copy link
Member

das-g commented Oct 27, 2019

looks like there's quite a bit of darwin failures in the fixup phase @NixOS/darwin-maintainers ? I think the patching of shebangs is causing it to fail

Is that in any way related to the change introduced by this PR?

@jonringer
Copy link
Contributor

jonringer commented Oct 28, 2019

probably not.... feel a little bad for darwin, then again.... I have no intentions of buying apple again.

@das-g das-g mentioned this pull request Oct 30, 2019
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FRidh FRidh Awaiting requested review from FRidh

3 more reviewers

@das-g das-g das-g approved these changes

@lsix lsix lsix approved these changes

@jonringer jonringer jonringer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bene1618 @das-g @lsix @jonringer