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

webbrowser: init at 29.0.0rc1 #88012

Merged
merged 2 commits into from Jun 26, 2020
Merged

webbrowser: init at 29.0.0rc1 #88012

merged 2 commits into from Jun 26, 2020

Conversation

TheBrainScrambler
Copy link
Contributor

@TheBrainScrambler TheBrainScrambler commented May 17, 2020

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • 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 nixpkgs-review --run "nixpkgs-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.
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.

@bbjubjub2494
Copy link
Member

Welcome!

You should add an entry to maintainers-list.nix with your info in it, that'll fix the evaluation error. Please commit on this PR. (Sidenote: I don't think OfBorg is going to go the full 2 hours but we'll see)

Shouldn't this be called palemoon? I don't use Pale Moon, but it seems that webbrowser isn't appropriate.

@TheBrainScrambler
Copy link
Contributor Author

Hello, thank you for you attention !
First, this isn't palemoon but another project, I just based my derivation on the palemoon derivation, and it seems I forgot to change the website.
Secondly, the tag wasn't here before, I just suggested today to the code's author to add them so that it becomes easier to maintain, and it seems he did this also today. So, yes, thank you for seeing that.
And finally, I will add myself to maintainers-list.nix, I didn't know that this existed.

@TheBrainScrambler
Copy link
Contributor Author

I think I'm done now. If I add gtk3 support then I will just open another PR.
This got reviewed, so I suppose it is ready to be merged ?

Copy link
Contributor

@puzzlewolf puzzlewolf left a 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 seds 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?

@TheBrainScrambler
Copy link
Contributor Author

TheBrainScrambler commented May 29, 2020

@puzzlewolf I'm not much used with Git, so I tried to squash and rename my commits using git rebase -i, I hope I did the right thing ...
And for all the seds, I think it is better to replace elements in the default config so that is works with NixOS instead of copying it and changing elements. This shows what we actually replace in the default config, and should be easier to maintain if the default config changes.
EDIT : and as you see, I fixed what you reviewed, thanks for that !

Copy link
Contributor

@puzzlewolf puzzlewolf 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 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:

  1. 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
  1. Use git commit --amend to change the commit message to maintainers: add TheBrainScrambler, then git rebase --continue to move to the next.
  2. Change the commit message of the second commit to webbrowser: init at 29.0.0rc1, git rebase --continue.
  3. 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?

Comment on lines 66 to 70
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
Copy link
Contributor

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 :)

@TheBrainScrambler
Copy link
Contributor Author

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 webbrowser-28.8.4.tar.gz or whatever archive from this tag and look at the content : it is the content of https://git.nuegia.net/webbrowser.git/tree/webbrowser and not https://git.nuegia.net/webbrowser.git/tree/ (noticed that there is no mach file ?). After seeing that I contacted the author of webbrowser so that he builds tags of the project root, not just containing the webbrowser directory, and so came the 29.0.0rc1. So in short, I can only package the 29.0.0rc1 version until he releases version 29.

@puzzlewolf
Copy link
Contributor

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 :)

@TheBrainScrambler
Copy link
Contributor Author

Ok I now use substituteInPlace instead of the seds, maybe just check if I didn't do any mistake there. And thanks for the git rebase tutorial, I'm still a git noob :D now it should be good.

@OPNA2608
Copy link
Contributor

OPNA2608 commented Jun 1, 2020

Hello, abit late but I figured I should chime in.

And for all the seds, I think it is better to replace elements in the default config so that is works with NixOS instead of copying it and changing elements. This shows what we actually replace in the default config, and should be easier to maintain if the default config changes.

The reasoning behind that in palemoon is:

  • The Linux build pages instruct us to create the file ourselves and fill in the settings we need, there doesn't appear to be a default config shipped (or I just wasn't able to find it).
  • We are compiling Palemoon with official branding so the build configuration with all our changes must be a-OK and ideally visible for upstream to verify at any point.

If this fork ships its default configuration and doesn't use official branding, then I think substituteInPlaceing the shipped configuration is prolly the better approach.


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 \ newline escapes all over the substituteInPlace command, that's why it currently fails.

    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.

@TheBrainScrambler
Copy link
Contributor Author

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.
And I fixed this stupid mistake, thank you ! As you may all have noticed, I'm regularly doing those, I am still a beginner ;)
NOW I suppose it should be good ?

@OPNA2608
Copy link
Contributor

OPNA2608 commented Jun 1, 2020

As you may all have noticed, I'm regularly doing those, I am still a beginner ;)

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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Build

@TheBrainScrambler
Copy link
Contributor Author

@OPNA2608 I thought grahamcofborg would help me to check for those errors, well, I see that it isn't the case.
I can't build it on my computer, it isn't powerful enough for this and would take too much time. When I wrote it before opening a PR I was using the Github CI, but this require to put some file in the fork which shouldn't appear on the PR. I think I will place it again and delete it once everything is really ok.

@OPNA2608
Copy link
Contributor

OPNA2608 commented Jun 2, 2020

I thought grahamcofborg would help me to check for those errors, well, I see that it isn't the case.
I can't build it on my computer, it isn't powerful enough for this and would take too much time.

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 configurePhase changes could easily have been tested with a nix-build, without having to go through the rest of the whole build process.

When I wrote it before opening a PR I was using the Github CI, but this require to put some file in the fork which shouldn't appear on the PR. I think I will place it again and delete it once everything is really ok.

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.

@TheBrainScrambler
Copy link
Contributor Author

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 ''.
As for Github Actions, I didn't know those also run on this repository, why does this happens ? And you say the bot can build the package, but does it allow to later get this build on my pc to test it ? With Github Actions I can use cachix-actions which allows that.
And finally, yes, I tested the build and webbrowser ran fine. I also tested video and a webgl game. It was already working before anyway, just needed to make sure those little changes didn't break anything.

--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"}"
Copy link
Member

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)?

Copy link
Contributor

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

Copy link
Contributor Author

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.

@TheBrainScrambler
Copy link
Contributor Author

Is everything ok, or did I miss something again ?

@TheBrainScrambler
Copy link
Contributor Author

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.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/200

@marsam marsam merged commit 3f338bb into NixOS:master Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants