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
webbrowser: init at 29.0.0rc1 #88012
Conversation
Welcome! You should add an entry to Shouldn't this be called |
Hello, thank you for you attention ! |
I think I'm done now. If I add gtk3 support then I will just open another PR. |
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.
Hey! Please review CONTRIBUTING.md and squash and change commit messages accordingly.
All those sed
s in the configurePhase
don't sit well with me 🤔. At a glance, neither firefox
nor palemoon
need them.
@AndersonTorres and @OPNA2608, you maintain palemoon
, which this package is based on. Could you take a look?
@puzzlewolf I'm not much used with Git, so I tried to squash and rename my commits using |
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 fixing that ;
:)
Yes, I think changing the config instead of writing a new one makes sense. I also found what I didn't like: There is substituteInPlace
, which is the nix way of doing this, please use it :)
Also, it seems that your git
maneuver didn't quite work out. I'd do it like this:
git rebase -i HEAD~8
, arrange it so it says
edit e4c24c5f01c Adding myself to maintainer-list.nix
edit abea6c0cb84 Updating nixpkgs
fixup 060e7610aef Change rev to the tag instzad of commit
fixup 5e36b4a6f83 Update sha256
fixup c42534d86b5 Changed homepage in meta, since this isn't palemoon
fixup 38e8c1d7809 Updated maintainer-list.nix
fixup 70665668448 Removed gtk3 option
fixup a3a44bebd40 Changed 'icon' in the desktopItem
- Use
git commit --amend
to change the commit message tomaintainers: add TheBrainScrambler
, thengit rebase --continue
to move to the next. - Change the commit message of the second commit to
webbrowser: init at 29.0.0rc1
,git rebase --continue
. - The other commits will be squashed into the second one without changing the message.
While writing I noticed something: We don't usually have non-stable releases in nixpkgs. This package should probably use version 28, and later update to 29. Do you mind changing the PR to use that?
sed -i "s:mk_add_options PYTHON=/usr/bin/python2:mk_add_options PYTHON=${python2}/bin/python2:g" $MOZCONFIG | ||
sed -i 's:mk_add_options AUTOCONF=/usr/bin/autoconf-2.13:mk_add_options AUTOCONF=${autoconf213}/bin/autoconf:g' $MOZCONFIG | ||
sed -i 's:ac_add_options --x-libraries=/usr/lib:ac_add_options --x-libraries=${lib.makeLibraryPath [ xorg.libX11 ]}:g' $MOZCONFIG | ||
sed -i 's:_BUILD_64=1:_BUILD_64=${lib.optionalString stdenv.hostPlatform.is64bit "1"}:g' $MOZCONFIG | ||
sed -i 's:mk_add_options MOZ_OBJDIR=/home/$USER/build/wbbuild/::g' $MOZCONFIG |
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 think substituteInPlace
(see the manual) would be more readable than sed
:)
Thank you for this in-depth review ! I will do all this when I can. As for the version, have a look at https://git.nuegia.net/webbrowser.git/ . There are currently two tags : 28.8.4 and 29.0.0rc1, so you might think that I can just use the 28.8.4 version. But now open the |
Hm. I don't know if it's ok then. Somebody else will have to merge this anyway, I don't have the privileges. They'll know :) |
Ok I now use |
Hello, abit late but I figured I should chime in.
The reasoning behind that in
If this fork ships its default configuration and doesn't use official branding, then I think I gave the derivation a quick look and overall it looks alright to me, but my head is currently in other projects right now. You're missing substituteInPlace $MOZCONFIG \
--replace "mk_add_options PYTHON=/usr/bin/python2" "mk_add_options PYTHON=${python2}/bin/python2" \
--replace "mk_add_options AUTOCONF=/usr/bin/autoconf-2.13" "mk_add_options AUTOCONF=${autoconf213}/bin/autoconf" \
--replace "ac_add_options --x-libraries=/usr/lib" "ac_add_options --x-libraries=${lib.makeLibraryPath [ xorg.libX11 ]}" \
--replace "_BUILD_64=1" "_BUILD_64=${lib.optionalString stdenv.hostPlatform.is64bit "1"}" \
--replace "mk_add_options MOZ_OBJDIR=/home/$USER/build/wbbuild/" "" That should get you through the current build problems. |
Hello, the fork has a default configuration and doesn't use official branding, it actually tries do not appear as a palemoon fork, so replacing things in the default config should be ok. |
No problem, we all started somewhere. 🙂 Can you try to test your changes locally first before you commit them? This page here has some good information on how to achieve that: https://nixos.wiki/wiki/Nixpkgs/Create_and_debug_packages#How_to_install_from_the_local_repository cd /path/to/your/nixpkgs/clone
git checkout your-branch
vi someFile.nix
nix-build . -A webbrowser 2>&1 | tee webbrowser.log
./result/bin/webbrowser is my usual workflow for testing build changes. Prolly not the best one out there but it's a start. |
@@ -0,0 +1,108 @@ | |||
{ stdenv, lib, fetchgit, makeDesktopItem, pkgconfig, makeWrapper | |||
# Build |
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.
# Build |
@OPNA2608 I thought grahamcofborg would help me to check for those errors, well, I see that it isn't the case. |
It does check for errors like this, and it's fine to use it for larger builds. I think it would've been preferable if you tried to test the build locally first though, just to see if the easy parts pass at all. Specifically the
The problem with that now is that you've triggered a GitHub Action build on both yours and this repository, and they appear to be much slower than the regular grahamcofborg builds (which have already passed awhile ago). Additionally, it seems the bot flagged your PR as a policy discussion, prolly for having that GitHub Action file. Build completed on the bot. I'll try to spin up a local build later to test if it runs fine as well. |
Yes, you are right, I should try to build locally for that. But the bot doesn't show all errors, since "checks passed" with it, although I was missing the ''. |
--replace "mk_add_options AUTOCONF=/usr/bin/autoconf-2.13" "mk_add_options AUTOCONF=${autoconf213}/bin/autoconf" \ | ||
--replace 'mk_add_options MOZ_OBJDIR=$HOME/build/wbobjects/' "" \ | ||
--replace "ac_add_options --x-libraries=/usr/lib64" "ac_add_options --x-libraries=${lib.makeLibraryPath [ xorg.libX11 ]}" \ | ||
--replace "_BUILD_64=1" "_BUILD_64=${lib.optionalString stdenv.hostPlatform.is64bit "1"}" |
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.
A small doubt:
If the platform is not 64 bits, this variable is set to 0 or just to "" (empty string)?
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.
https://developer.palemoon.org/build/linux/
# Clear this if not a 64bit build
_BUILD_64=1
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.
And this comment is also in https://git.nuegia.net/webbrowser.git/tree/doc/mozconfig.example , so it should be good.
Is everything ok, or did I miss something again ? |
Well, I've deleted the workflow file. If I didn't miss something it should be ready to merge. By deleting the commit which added the workflow file, nix-build still used the cachix build, and webbrowser still runs, so should be good. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Motivation for this change
Webrowser is, as its name suggests, a web browser. It is actually my first derivation, so mistakes could have been done. I made it as sort of training, but since it works I'm motivated to publish it here and to try to maintain it.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)Additionnal notes
Note that I am also not very used to git and Github.
This derivation was built using Github Actions, and ran on NixOS. The build can take a long time, on Github Action it was usually between 1h30 and 2h.
I removed the commit history, since I had 60 commit with many many useless ones to trigger Github Actions, and because I've heard that we should do that to clean the nixpkgs commit history.
Important : there is a withGTK3 option, but when I tested it the package was built using GTK2. I will maybe try to solve this issue, but it isn't a priority to me, so I'm already opening a PR.
EDIT : removed the withGTK3 option, as said above I will maybe implement it later.