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
webmacs: init at version 0.8 #69088
webmacs: init at version 0.8 #69088
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for choosing to contribute to nixpkgs, looks like an interesting program.
I've put in a few comments to bring the style closer to other packages in nixpkgs
Also at some point you will want to rebase so that you have 2 commits
- The first adding yourself to the maintainers list titled
maintainers: add jacg
- The second adding the package titled
webmacs: init at 0.8
platforms = platforms.all; | ||
}; | ||
|
||
preBuild = "CC=g++"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right. Nix should normally handle setting up your C compiler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yes ... it needs to use a C++ rather than a C compiler. Any hints on how else to persuade it to do so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build fails with gcc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup:
gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/build/source/vendor/bloom-filter-cpp -I/build/source/vendor/hashset-cpp -I/build/source/vendor/ad-block -I/nix/store/96p42644i76jqgdkxjgyl3f0c8n8k16j-python3-3.7.4/include/python3.7m -c /build/source/c/adblock.c -o build/temp.linux-x86_64-3.7/build/source/c/adblock.o -g0 -std=c++11
cc1: warning: command line option ‘-std=c++11’ is valid for C++/ObjC++ but not for C
/build/source/c/adblock.c:19:10: fatal error: iostream: No such file or directory
#include <iostream>
^~~~~~~~~~
compilation terminated.
error: command 'gcc' failed with exit status 1
builder for '/nix/store/lb8cqs8ypcsb94mj85r7k1kv1s73r51b-webmacs-0.8.drv' failed with exit code 1
... which is to be expected, when compiling C++ programs with a C compiler (unless you explicitly tell it where to find the C++ standard libraries ... but that's kinda the point of g++: it takes care of those sorts of things).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel it should be using $CXX but I don't know what you can do if it doesn't (apart from what you have done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly do you mean by this (I'm still a little lost in nix terminology)? If I try putting CC = "g++";
at the same level as pname = "webmacs;
then it fails in exactly the same way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is what I meant but clearly it doesn't work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leave it as is for now, It is probably me being picky about something that is actually ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me poke around a bit to try to understand why it's using $CC
rather than $CXX
, but if I don't learn anything in a reasonably short amount of time, then I'll leave it as is. In the meantime I'll push all the other changes you requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever the result of this, this should go a bit further up in the file and not be hidden down here
@alexrice Thank you very much for taking the time to look at this and provide the helpful review comments. I've pushed the rebased branch with all (I hope) the changes you requested except the |
any ideas? |
Hmm ... I'm not seeing that here (which is exactly the sort of thing that isn't supposed to happen with nix!). How should I be testing this reliably, locally?
certainly doesn't give me the problem you observe. (Also, when I try to break it deliberately, for example, by changing the revision hash, it happily gives me whatever it built before, which is worrying.) |
Changing the revision hash will not work as |
I get the problem after running |
I've run it in a pure shell, it's the same hash as the one you observe ... what else can I try to reproduce the error? |
It builds fine, it is running the program which causes issue |
It looks like the use of |
I cannot work this out either. I believe you want to follow the advice in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the issue, you need pyqtwebengine
instead of pyqt5_with_qtwebkit
See https://stackoverflow.com/questions/51154871/python-3-7-0-no-module-named-pyqt5-qtwebenginewidgets
One thing, before I forget:
This is disturbing. |
b4af821
to
ef850cc
Compare
This is probably lost in the mess of messages above but can the maintainer commit be called "maintainers: add jacg" |
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more small cleanup changes but this builds and runs with nix-review
I hope this hasn't been too traumatic. Most packages are far easier to package than this.
platforms = platforms.all; | ||
}; | ||
|
||
preBuild = "CC=g++"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever the result of this, this should go a bit further up in the file and not be hidden down here
@GrahamcOfBorg build |
Done.
I held off submitting it for ages, fearing the process, but your help and patience has made perfectly bearable. Thank you very much. |
If the |
I would leave it for now. Someone else will have to take a look at this as well as I do not have commit rights. They may have an opinion on it |
@alexarice I get the impression that this PR is simply adding to the growing backlog of over 1000 open PRs. If the committers are overwhelmed by the sheer volume of PRs, perhaps it's best just to close this? There are more important things to be done than getting this package in. |
I think you should leave it open. If no one gets round to looking at it then it does not burden committers and if they do get round to looking at it then it can get merged. You could also consider making an NUR repo if you want to make it available sooner |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/59 |
@GrahamcOfBorg build webmacs |
1 similar comment
@GrahamcOfBorg build webmacs |
@alexarice Is correct on this @jacg, eventually a committer will get around to looking at your change. Them actually asking in discourse helped grab my attention at least. This particular package has been reviewed very well, and I've pushed any nitpicks and made |
3bd3b02
to
bcecdd5
Compare
Motivation for this change
This package was not available in nixpkgs, before.
(The check phase is disabled at present, at is depends on a number of things that are not available in nixpkgs. I plan to contribute these as well, but wanted to keep my first contribution down to a manageable and easily-reviewable size.)
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @