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.

Sign up for GitHub

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

Jump to bottom

Repaired pythonModules.qscintilla-qt5 #71641

Merged
merged 1 commit into from
Oct 28, 2019
Merged

Repaired pythonModules.qscintilla-qt5 #71641

merged 1 commit into from
Oct 28, 2019
+2 −1

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

Sorry, something went wrong.

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 requested a review from lsix October 22, 2019 09:00
lsix
lsix requested 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. 👍

Sorry, something went wrong.

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

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 

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

Thank you, I squashed the two commits.

@jonringer
Copy link
Contributor

@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

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

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

@jonringer jonringer merged commit b27bdf4 into NixOS:master Oct 28, 2019
@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

@das-g das-g

@lsix lsix

@jonringer jonringer

@FRidh FRidh

Assignees
No one assigned
Labels
6.topic: python 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bene1618 @das-g @lsix @jonringer