MediaHarbor: init at 2.1.0 - #550041
Conversation
d37f8be to
3a27d8a
Compare
nixpkgs-ci
Bot
added
8.has: package (new)
RossSmyth
left a comment
RossSmyth
left a comment
There was a problem hiding this comment.
Good start. Does this need cargo-tauri or does it work fine without it?
| zlib, | ||
| ... | ||
| }: | ||
|
|
There was a problem hiding this comment.
Don't need a wildcard, the exact requested packages will always be supplied.
| zlib, | |
| ... | |
| }: | |
| zlib, | |
| }: | |
| ... | ||
| }: | ||
|
|
||
| rustPlatform.buildRustPackage rec { |
There was a problem hiding this comment.
Please use finalAttrs, and not rec as it integrates with the override system, while rec does not.
| rustPlatform.buildRustPackage rec { | |
| rustPlatform.buildRustPackage (finalAttrs: { |
| }: | ||
|
|
||
| rustPlatform.buildRustPackage rec { | ||
| pname = "MediaHarbor"; |
There was a problem hiding this comment.
Should usually match the attribute name, so it should be lowercase.
| pname = "MediaHarbor"; | |
| pname = "mediaharbor"; |
| src = fetchFromGitHub { | ||
| owner = "MediaHarbor"; | ||
| repo = "mediaharbor"; | ||
| rev = "v${version}"; |
There was a problem hiding this comment.
Please use tag, so using rev gives GitHub a chance to fetch the incorrect thing (if shortened hashes line up with a tag name)
| rev = "v${version}"; | |
| tag = "v${version}"; |
There was a problem hiding this comment.
Looks like there is a Cargo.lock upstream, so this shouldn't be needed.
There was a problem hiding this comment.
should i keep the lockFIle = ./Cargo.lock or will that cause issues? I feel like keeping its correct but I would like to minimize rebuilding this package.
There was a problem hiding this comment.
No, please remove the vendored lockfile.
| cargoBuildFlags = [ | ||
| "--manifest-path" | ||
| "src/app/Cargo.toml" | ||
| ]; |
There was a problem hiding this comment.
Rather than manifest-path this should be "-p" "your-crate-here"
| PKG_CONFIG_PATH = lib.makeSearchPath "lib/pkgconfig" [ | ||
| atk.dev | ||
| cairo.dev | ||
| dbus.dev | ||
| gdk-pixbuf.dev | ||
| glib.dev | ||
| gtk3.dev | ||
| harfbuzz.dev | ||
| libsoup_3.dev | ||
| pango.dev | ||
| webkitgtk_4_1.dev | ||
| zlib.dev | ||
| ]; |
There was a problem hiding this comment.
This is done for you. No need to specify the dev outputs either. Just place these packages in buildInputs and it should just work as long as pkg-config is in nativeBuildInputs.
There was a problem hiding this comment.
For some reason it would not build without manually adding them to the path, I wasted like 2 hours just adding them to buildinputs and removing them, package config just could not find them unless manually adding them with the .dev as well, ill try building without again, but this may need to stay.
There was a problem hiding this comment.
If that is the case then there is something else wrong.
There was a problem hiding this comment.
I changed it myself and it works fine.
| npm run build:react -- --outDir dist-react | ||
| ''; | ||
|
|
||
| meta = with lib; { |
There was a problem hiding this comment.
Please don't use with lib;, there is an ongoing intiative to remove this in nixpkgs as it has a measurable eval slowdown across all packages.
| meta = with lib; { | |
| meta = { |
| nativeBuildInputs = [ | ||
| nodejs | ||
| npmHooks.npmConfigHook | ||
| npmHooks.npmInstallHook |
There was a problem hiding this comment.
npmInstallHook overrides the install phase. Does the package still install properly? If not disable the override (check the nixpkgs reference) and put it in pre or post install.
| description = "Cross-platform Media Ripping and Browsing GUI"; | ||
| homepage = "https://mediaharbor.org/"; | ||
| license = licenses.gpl3Only; | ||
| mainProgram = "mediaharbor"; |
There was a problem hiding this comment.
This package will not be merged without a maintainer.
|
re: maintainers list in a seperate PR just put yourself on the maintainers list in one PR, no need for two. |
|
Im currently rebuilding for other reasons (I forgot to add a desktop file) but it does seem to work quite well, but it will probably take a few hours for me to get anything done as it takes 30 minutes for me to build the package (old intel laptop :/) |
nixpkgs-ci
Bot
removed
the
9.needs: reviewer
|
Building it myself, yes |
RossSmyth
left a comment
RossSmyth
left a comment
There was a problem hiding this comment.
Some other things I noticed while messing with it.
I also noticed it has some optional runtime dependencies
- Python
- Bento4
So those can be provided if you want to.
| typescript | ||
| wrapGAppsHook3 | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Also need libgit2 in buildInputs and env.LIBGIT2_NO_VENDOR = true so that Nixpkgs libgit2 is used and not the vendored one
| wrapGAppsHook3 | ||
| ]; |
There was a problem hiding this comment.
This is needed for proper dependency propagation. This is also how it is built upstream, and we should replicate what upstream does as close as we can.
| wrapGAppsHook3 | |
| ]; | |
| wrapGAppsHook3 | |
| cargo-tauri.hook | |
| ]; |
| zlib | ||
| ]; |
There was a problem hiding this comment.
This package also requires the base gst plugins or else it gets unhappy
| zlib | |
| ]; | |
| zlib | |
| gst_all_1.gst-plugins-base | |
| ]; |
There was a problem hiding this comment.
alright, i just woke up, ill probably spend like 5 hours on it today (unless stuff goes really well) thx for the help :3
3a27d8a to
abe2f5d
Compare
2 participants
Things done
Fetched and built MediaHarbor package. - Pulled Cargo.lock and github repo
(probably not the best way to do it but wtvr) and built with rustPlatform.buildRustPackage.
NOTE : i have not added any maintainers or maintainer list related stuff yet
because I have another active PR with the maintainer list, if this is not what I
be doing, please mention so. thx :3
passthru.tests.nixpkgs-reviewon this PR. See [nixpkgs-review usage]../result/bin/.