-
-
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
aether: init at 2.0.0-dev.15 #130105
aether: init at 2.0.0-dev.15 #130105
Conversation
Whoever reviews this: This is my first new package contribution, so I don't expect to have done everything right. I don't mind direct critique. <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.
You're off to a good start.
Please update the commit messages to conform to the guidelines mentioned in the pr template. You could use git rebase -i
and the r
(reword) rebase command to achieve this.
Result of 1 package built successfully:
2 suggestions:
|
@SuperSandro2000 @roberth How exactly is the merge process going to happen (ie. who pushes the merge button)? I am asking because I want to address your review comments with single commits (otherwise Github gets confused IIRC) but still squash those into the "package init" commit before in lands on master. |
A member can either merge it as it is in the PR or squash merge it for you. Just remember that adding yourself as a maintainer and init of the package should be 2 separate commits :) |
@roberth @SuperSandro2000 as far as I can tell I addressed the suggestions. I also added some more code because I had to get the app icon from their git repository. I hope the force push did not throw off Github too much (happens some time). Regarding the license I wrote a request at their bugtracker[1] (My post is still pending moderation). |
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 good to me.
Regarding the use of string constants, I don't expect most of them stand the test of an actual change happening, and the indirection could be slightly harmful. version
is a notable exception. I don't feel very strongly about it though, so I wouldn't delay a merge for it.
Maybe @SuperSandro2000 wants to have another look. Otherwise, good for merge afaic.
@roberth @SuperSandro2000 I squashed the package commits together, so from my POV this is ready to merge. Regarding the strings constant I admit it is a bit pointless as of now, eg. you will find If the Aether project stays alive I assume we will rework this derivation to build from source at some point anyway. |
@maxhille I agree. The build did fail to fetch the archive though. That's hard to test locally because fixed-output derivation outputs are always cached. You can delete the cached paths from your store using
If deletion fails, you may have to remove gc roots:
|
Turns out that missing dot was my fault somehow. Fixed now
Thanks for the pointer. FYI I had to do |
Ah that makes sense when run in the aether directory. I assumed to run from the nixpkgs root. |
I guess that also explains why I had to remove the |
desktopItem = makeDesktopItem { | ||
name = pname; | ||
exec = binaryName; | ||
icon = pname; | ||
desktopName = "Aether"; | ||
genericName = meta.description; | ||
categories = "Network;"; | ||
mimeType = "x-scheme-handler/aether"; | ||
}; |
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.
Use copyDesktopItems instead.
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 you give a reason why I should do that?
Unfortunately I have serious trouble to find the sources for the imported functions, so I don't know more about copyDesktopItems
other than that it does not work as a substitute because some argument is missing...
Also please note that the original tarball does not come with a desktop file, so we have to create one. As said I don't know what copyDesktopItem
does but I assume is copies existing files?
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 you give a reason why I should do that?
Because it is supposed to be done this way instead of custom cooking the same install steps in all packages that use it.
here you have an example
nixpkgs/pkgs/applications/graphics/antimony/default.nix
Lines 41 to 59 in 2828164
nativeBuildInputs = [ cmake flex lemon wrapQtAppsHook copyDesktopItems ]; | |
desktopItems = [ | |
(makeDesktopItem { | |
name = "antimony"; | |
desktopName = "Antimony"; | |
comment="Tree-based Modeler"; | |
genericName = "CAD Application"; | |
exec = "antimony %f"; | |
icon = "antimony"; | |
terminal = "false"; | |
categories = "Graphics;Science;Engineering"; | |
mimeType = "application/x-extension-sb;application/x-antimony;"; | |
extraEntries = '' | |
StartupWMClass=antimony | |
Version=1.0 | |
''; | |
}) | |
]; |
don't know what copyDesktopItem does but I assume is copies existing files?
It can also copy created items, see the example above.
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 the explanation.
In contrast to the example, I had to add runHook postInstall
to the installPhase
block, otherwise copyDesktopItems
would just not execute.
Again I have no idea why that is...
I added some formatting suggestions and changed the license to |
@roberth Seem like a check for some inferior, proprietary OS got stuck. Is there something I can do about it? |
The problem is with our tooling. To be honest, I don't know the exact status of the ofborg x86_64-darwin builders, but I'm pretty sure it's not because of the operating system. You can ignore the darwin commit status because you didn't package for it and
Please don't make a fool of yourself. Also, complacency is a real problem for open source if "it" wants to compete in the OS arena. It's not the root cause though and I won't blame any individual for it. But hey, let's enjoy ourselves :) |
@maxhille Thanks for your persistence here in diligently updating to feedback. It's looking like a great first package. |
@SuperSandro2000 Can you revisit the suggestions / changes I made? |
postUnpack = '' | ||
cp source/aether-core/aether/client/src/app/ext_dep/images/Linux-Windows-App-Icon.png ${sourceRoot} | ||
''; | ||
|
||
buildPhase = '' | ||
convert Linux-Windows-App-Icon.png -resize 512x512 aether.png |
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.
postUnpack = '' | |
cp source/aether-core/aether/client/src/app/ext_dep/images/Linux-Windows-App-Icon.png ${sourceRoot} | |
''; | |
buildPhase = '' | |
convert Linux-Windows-App-Icon.png -resize 512x512 aether.png | |
buildPhase = '' | |
convert source/aether-core/aether/client/src/app/ext_dep/images/Linux-Windows-App-Icon.png -resize 512x512 aether.png |
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. had to add relative ../source
though.
basically cloned the discord package as it is also electron
Hi all — maintainer of Aether P2P here. I've been out travelling for the past few weeks, so I'm just seeing all this. Great to see this work out. Let me know if you have any questions and/or requests. Once this is done and working, I'd be happy to add NixOS as officially supported and point it your way. |
In a couple of days it should be available on the unstable channel. |
is the binary release, so this package should be called the package ... no? |
We only do this if we have both the source and the bin variant. Ideally if we build from source the bin variant can be removed. |
edit: forget what i said ...
so lets build from source in the first place ... the biggest problem:
aether is oldprobably the devs focus on the web version the last serious commit was two years ago (53b6c8b from 2019-09-20) only 9 commits in the git repo ... so this looks not like something "we" want in nixpkgs, apart from that: backend and frontend are go modules, frontend
but the frontend will store 700MB of something in clientelectron shows an empty window
the client is a javascript/vue/electron app
but its easy to build the client with npm:
we dont need the original bundling (compress with upx, create docker image) |
Last commit is from July 21, 2021, I doubt that counts as abandonware, it's clearly being tended to. On the other hand, it is true right now focused on the web version because we've just released it. If you are going to build it from source, I would recommend you to wait until we cut the dev16, which should happen in a week or two. We're fixing a low-level CVE where a local user can attack its own app and that will be the main release. |
This comment has been minimized.
This comment has been minimized.
@nehbit is there a link to the dev16 release? |
The changes in this PR add myself to the maintainer list and the Aether P2P client as a package.
about Aether: https://getaether.net/
Motivation for this change
No package yes. People were asking for it (#66414)
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)