-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
rtfm: init at 0.2.2 #244326
rtfm: init at 0.2.2 #244326
Conversation
@drupol, @SuperSandro2000 could you please review this PR? It's been 3 days and no one wants to :( |
stdenv.mkDerivation (finalAttrs: { | ||
name = "gtk-docs"; | ||
|
||
adw1_ver = "1.3.3"; |
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.
adw1_ver = "1.3.3"; | |
adw1_version = "1.3.3"; |
adw1_docs = fetchFromGitHub { | ||
owner = "GNOME"; | ||
repo = "libadwaita"; | ||
rev = "refs/tags/${finalAttrs.adw1_ver}"; |
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.
rev = "refs/tags/${finalAttrs.adw1_ver}"; | |
rev = "refs/tags/${finalAttrs.adw1_version}"; |
hash = "sha256-YIxGwl+/F7xkGjoi07GViSHAfCTE1RpEBhHfrlD0X/4="; | ||
}; | ||
|
||
gtk4_ver = "4.11.4"; |
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.
gtk4_ver = "4.11.4"; | |
gtk4_version = "4.11.4"; |
gtk4_docs = fetchFromGitHub { | ||
owner = "GNOME"; | ||
repo = "gtk"; | ||
rev = "refs/tags/${finalAttrs.gtk4_ver}"; |
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.
rev = "refs/tags/${finalAttrs.gtk4_ver}"; | |
rev = "refs/tags/${finalAttrs.gtk4_version}"; |
hash = "sha256-YobWcLJm8owjrz6c6aPMCrVZqYDvNpjIt5Zea2CtAZY="; | ||
}; | ||
|
||
pango_ver = "1.50.14"; |
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.
pango_ver = "1.50.14"; | |
pango_version = "1.50.14"; |
pango_docs = fetchFromGitHub { | ||
owner = "GNOME"; | ||
repo = "pango"; | ||
rev = "refs/tags/${finalAttrs.pango_ver}"; |
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.
rev = "refs/tags/${finalAttrs.pango_ver}"; | |
rev = "refs/tags/${finalAttrs.pango_version}"; |
}; | ||
|
||
patches = [ | ||
./make.patch |
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.
Could you please add a comment on why this patch is required, for each patch in this list?
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.
Sure
ln -s ${gtk-docs} gtk-docs | ||
''; | ||
|
||
meta = with lib; { |
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.
Could you please add the platforms
attribute with proper values? So we know on which architecture this software is available.
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.
Can i set platforms = [ "i686-linux" "x86_64-linux" ];;
because i can't test on aarch64
and darwin
?
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.
Just set the platforms the software you're trying to add supports, then the CI will test on these architectures.
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 set platforms.linux
. Theoretically, it might run on darwin
, maybe i'll setup OpenCore later on my PC.
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.
Or you can ask to the Darwin maintainers team, they are quite reactive. Set it correctly and we'll ask them.
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.
Done. if I understand correctly how to do it
platforms = platforms.linux ++ platforms.darwin;
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.
Yes!
785ab0d
to
60fb6d6
Compare
cp -r * $out | ||
|
||
runHook postInstall | ||
''; |
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 should also get a meta section
buildInputs = [ gobject-introspection libadwaita gtk4 pango ]; | ||
|
||
doCheck = false; | ||
doInstallCheck = false; |
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.
doInstallCheck = false; |
only one should be required here
dontUnpack = true; | ||
|
||
nativeBuildInputs = [ gi-docgen ]; | ||
buildInputs = [ gobject-introspection libadwaita gtk4 pango ]; |
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.
gobject-introspection goes to nativeBuildInputs
gtk4_version = "4.11.4"; | ||
gtk4_docs = fetchFromGitHub { | ||
owner = "GNOME"; | ||
repo = "gtk"; | ||
rev = "refs/tags/${finalAttrs.gtk4_version}"; | ||
hash = "sha256-YobWcLJm8owjrz6c6aPMCrVZqYDvNpjIt5Zea2CtAZY="; | ||
}; | ||
|
||
pango_version = "1.50.14"; | ||
pango_docs = fetchFromGitHub { | ||
owner = "GNOME"; | ||
repo = "pango"; | ||
rev = "refs/tags/${finalAttrs.pango_version}"; | ||
hash = "sha256-b3N9kM8xZ/FkpPR9gYJz96tiG/XUOUB6ipfLmBllCKc="; | ||
}; |
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.
Do we really must pin those 3?
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 so, because they are marked as dependencies on the project's github, that is, the author assumes that during the process of building the program, all the documentation is present in the system.
- Offline crystal api documentation for Crystal docset generation.
- Offline Gtk api documentation for Gtk4, Gdk4, Gsk4, libAdwaita and Pango libraries for GTK4 libraries docset generation.
We can build a program without documentation at all, but in this case rtfm
will be useless. It also makes no sense to set part of the documentation, since in this case the user himself will have to generate docsets from sources.
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 didn't mean that we could remove them but if we could use the versions from nixpkgs.
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 didn't find docs in gtk/pango/libadwaita nix packages.
, pango | ||
}: | ||
stdenv.mkDerivation (finalAttrs: { | ||
name = "gtk-docs"; |
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.
It should get pname+version even if version is just dummy or from rtfm or so
description = "Read the Formidable Manual is a dash/docset reader with built in documentation for Crystal and GTK APIs."; | ||
homepage = "https://github.com/hugopl/rtfm/"; | ||
license = licenses.mit; | ||
mainProgram = "rtfm"; |
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.
mainProgram = "rtfm"; |
that's already the default
homepage = "https://github.com/hugopl/rtfm/"; | ||
license = licenses.mit; | ||
mainProgram = "rtfm"; | ||
platforms = platforms.linux ++ platforms.darwin; |
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.
platforms = platforms.linux ++ platforms.darwin; |
please check if buildCrystalPackage implies this
doCheck = false; | ||
|
||
preBuild = '' | ||
tar -xf ${crystal-docs} && mv crystal-${crystal_doc_version}-docs/ crystal-docs/ |
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.
tar -xf ${crystal-docs} && mv crystal-${crystal_doc_version}-docs/ crystal-docs/ | |
tar -xf ${crystal-docs} | |
mv crystal-${crystal_doc_version}-docs/ crystal-docs/ |
, gi-crystal | ||
}: | ||
let | ||
gtk-docs = callPackage ./gtk-docs.nix { }; |
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.
Should we passthru this?
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.
Like this?
let
passthru = {
gtk-docs = callPackage ./gtk-docs.nix { };
};
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.
normally we inherit into passthru so that overriding passthru is not changing things
127f6dc
to
68d72f7
Compare
@hugopl could you help with one point? I generate Is there a way to change [dependencies."GObject-2.0"]
name = "GObject"
description = "The base type system library"
docs_url = "https://docs.gtk.org/gobject/"
[dependencies."Gio-2.0"]
name = "Gio"
description = "GObject Interfaces and Objects, Networking, IPC, and I/O"
docs_url = "https://docs.gtk.org/gio/"
[dependencies."cairo-1.0"]
name = "cairo"
description = "A 2D graphics library with support for multiple output devices"
docs_url = "https://www.cairographics.org/manual/"
... |
After force-pushing last commit, i noticed some checks were failed.
error: attribute 'devdoc' missing
at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/ofborg-evaluator-7/pkgs/applications/misc/rtfm/default.nix:67:20:
66|
67| for file in "${gtk4.devdoc}"/share/doc/*; do
| ^
68| ln -s "$file" "gtk-doc/$(basename "$file")" It happens because outputs = [ "out" "dev" ] ++ lib.optionals x11Support [ "devdoc" ]; So, maybe i need to override |
seems like it |
Nothing happened, i don't know why and how to fix it |
substituteInPlace Makefile --replace "crystal run src/create_crystal_docset.cr" "crystal src/create_crystal_docset.cr ${crystal}/share/doc/crystal/api/" | ||
substituteInPlace Makefile --replace "crystal run src/create_gtk_docset.cr" "crystal src/create_gtk_docset.cr gtk-doc/" |
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.
substituteInPlace Makefile --replace "crystal run src/create_crystal_docset.cr" "crystal src/create_crystal_docset.cr ${crystal}/share/doc/crystal/api/" | |
substituteInPlace Makefile --replace "crystal run src/create_gtk_docset.cr" "crystal src/create_gtk_docset.cr gtk-doc/" | |
substituteInPlace Makefile \ | |
--replace "crystal run src/create_crystal_docset.cr" "crystal src/create_crystal_docset.cr ${crystal}/share/doc/crystal/api/" \ | |
--replace "crystal run src/create_gtk_docset.cr" "crystal src/create_gtk_docset.cr gtk-doc/" |
I can reproduce this on Archlinux too and created an issue. |
webkitgtk_6_0 | ||
sqlite | ||
libadwaita | ||
(gtk4.override { x11Support = true; }) |
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.
(gtk4.override { x11Support = true; }) | |
gtk4' |
, libadwaita | ||
, gtk4 | ||
, pango | ||
}: |
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 | |
gtk4' = gtk4.override { x11Support = true; }; | |
in |
for file in "${gtk4.devdoc}"/share/doc/*; do | ||
ln -s "$file" "gtk-doc/$(basename "$file")" | ||
done | ||
|
||
for file in "${pango.devdoc}"/share/doc/*; do | ||
ln -s "$file" "gtk-doc/$(basename "$file")" | ||
done | ||
|
||
for file in "${libadwaita.devdoc}"/share/doc/*; do |
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.
for file in "${gtk4.devdoc}"/share/doc/*; do | |
ln -s "$file" "gtk-doc/$(basename "$file")" | |
done | |
for file in "${pango.devdoc}"/share/doc/*; do | |
ln -s "$file" "gtk-doc/$(basename "$file")" | |
done | |
for file in "${libadwaita.devdoc}"/share/doc/*; do | |
for file in "${gtk4'.devdoc}"/share/doc/*; do | |
ln -s "$file" "gtk-doc/$(basename "$file")" | |
done | |
for file in "${pango.devdoc}"/share/doc/*; do | |
ln -s "$file" "gtk-doc/$(basename "$file")" | |
done | |
for file in "${libadwaita.devdoc}"/share/doc/*; do |
Those are taken directly from the inputs, not buildInputs where you did the override
@SuperSandro2000 it works, all checks passed |
@sund3RRR would you mind in creating a PR for rtfm README with the instructions to install this NixOS package? |
Sure, but these instructions will be useless until this PR is approved |
Yeah, sure, I mean, when it gets approved/merged. |
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 your contribution, it's ok for me now.
|
||
patches = [ | ||
# 1) fixed gi-crystal binding generator command | ||
# 2) removed `-v` arg to `cp` command to prevent build failure due to stdout buffer overflow |
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 can fix that upstream.
# 1) fixed gi-crystal binding generator command | ||
# 2) removed `-v` arg to `cp` command to prevent build failure due to stdout buffer overflow | ||
# 3) added commands to build gschemas and update icon-cache | ||
./patches/make.patch |
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.
Usually distros run these command for any packages that install files on specific dirs used by these commands.
configure: | ||
- shards install | ||
- ./bin/gi-crystal | ||
+ gi-crystal |
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.
Why you guys don't use the gi-crystal that the project depends on? Do you guys have a gi-crystal nix package? if so, why?
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 packed gi-crystal
in nixpkgs
when I packed the collision
. You can see these PR's: #243914
#242681
There is an explanation why we decided to do it this way.
Shortly, nix collects crystal dependencies in separated derivations, then creates a symlimks on it in lib/ directory of project. I am creating a patch when packing gi-crystal
to make it work with symlinks. The main problem is that all derivations in nix store are read-only and changes in one derivation will change drv hash and all hashes, that depend on it. When we run ./bin/gi-crystal
it fails with error Permission denied
, because it cannot write to read-only directory.
So, gi-crystal
in nixpkgs works differently due to this nix features. All you need to do is add gi-crystal
in build inputs, then run gi-crystal
. It will scan lib/
directory with all symlinks, then generate all bindings in lib/gi-crystal
directory and copy self package to this.
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.
Shortly, nix collects crystal dependencies in separated derivations, then creates a symlimks on it in lib/ directory of project. I am creating a patch when packing
gi-crystal
to make it work with symlinks. The main problem is that all derivations in nix store are read-only and changes in one derivation will change drv hash and all hashes, that depend on it. When we run./bin/gi-crystal
it fails with errorPermission denied
, because it cannot write to read-only directory.
Jumping into the convo here... How about copying the gi-crystal
derivation into the current rtfm directory and then use it ? That would fix both issues. You will be re-using the existing gi-cristal
and it's directory is writable since you copied it into the rtfm
build directory.
WDYT? Is it feasible?
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.
How about copying the gi-crystal derivation into the current rtfm directory and then use it ?
This is exactly what I did when packing gi-crystal
. But I actually automated this action so as not to do it constantly for different packages.
I think that's better, don't you?
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 I mean is to copy the output of gi-crystal
into rtfm
and then use it.
Once the compilation is over, you delete it and only copy the relevant rtfm
files in $out
.
WDYT?
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 I mean is to copy the output of gi-crystal into rtfm and then use it.
Once the compilation is over, you delete it and only copy the relevant rtfm files in $out.
I'm sorry but I don't understand what's the difference. If we want to make it work without patching Makefile
, then we need to do several steps:
mkdir lib/gi-crystal
cp -r ${gi-crystal}/* lib/gi-crystal
mkdir bin/
mv lib/gi-crystal/bin/gi-crystal bin/
chmod +w lib/gi-crystal/src/auto
I think it is just unnecessary steps to be done on each package requires gi-crystal.
And even if we do that, we still have to patch this to remove shards install
command, because it will crash building due to cannot fetch remote host github.com
something like this, I definitely don't remember what that error sounds like.
WDYT?
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 don't see the issue to do that all the time. I prefer that than patching the src.
But anyway, I was just passing by here thinking to provides new ideas, but I see that you already studied the question so I won't bother. I never used gi-crystal this is the reason why I might be wrong too.
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 don't see the issue to do that all the time. I prefer that than patching the src.
But anyway, I was just passing by here thinking to provides new ideas, but I see that you already studied the question so I won't bother. I never used gi-crystal this is the reason why I might be wrong too.
Thanks for suggesting solutions to this problem. Patching sources was the reason why I didn't want to pack gi-crystal
. My best idea is to make gi-crystal
portable, this fits well with the nix ideology, but changes program behaviour.
Speaking of using gi-crystal, I understand the steps involved in implementing your idea. But others may not realize that you need to copy the gi-crystal
directory, mv gi-crystal
binary and change the write permissions.
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.
Ok... sad but ok.
It's weird that it needs to write in the gi-crystal
directory.
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.
Looks like you guys are re-doing what shards already does, but at a package level. I mean... if derivations means a packaged code.
- cp -rv data/Crystal.docset $(DESTDIR)$(PREFIX)/share/rtfm/docsets/ | ||
- cp -rv data/Gtk4.docset $(DESTDIR)$(PREFIX)/share/rtfm/docsets/ | ||
+ cp -r data/Crystal.docset $(DESTDIR)$(PREFIX)/share/rtfm/docsets/ | ||
+ cp -r data/Gtk4.docset $(DESTDIR)$(PREFIX)/share/rtfm/docsets/ |
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 added this change upstream at hugopl/rtfm@8833114
install -D -m0644 CHANGELOG.md $(DESTDIR)$(PREFIX)/share/doc/rtfm/CHANGELOG.md | ||
gzip -9fn $(DESTDIR)$(PREFIX)/share/doc/rtfm/CHANGELOG.md | ||
+ gtk4-update-icon-cache --ignore-theme-index $(PREFIX)/share/icons/hicolor | ||
+ glib-compile-schemas $(DESTDIR)$(PREFIX)/share/glib-2.0/schemas |
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 can't add this upstream or it will break ArchLinux package.
|
||
# added chmod +w for copied docs to prevent error: | ||
# `Error opening file with mode 'wb': '.../style.css': Permission denied` | ||
./patches/enable-write-permissions.patch |
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.
Could you explain this error? why does it happen? So I can fix it upstream too.
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.
It happens because rtfm
copies docs from nix derivation, but all files in nix-store are read-only, so at first we need to add write permissions to the copied file. This error is nix-specific, but it cannot break rtfm
on other distros i think.
Description of changes
It's a dash/docset reader with built in documentation for Crystal and GTK APIs. It's written in Crystal using GTK4 bindings.
https://github.com/hugopl/rtfm
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)