-
-
Notifications
You must be signed in to change notification settings - Fork 17.6k
gologin: init at 4.0.2 #476954
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
base: master
Are you sure you want to change the base?
gologin: init at 4.0.2 #476954
Conversation
Hythera
left a comment
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 have to call it package.nix instead of default.nix, since the package is located under pkgs/by-name.
|
Ohh i see thanks |
|
|
Anything else i can do from my side? |
|
I just want to mention that since this is a browser (I think), it's going to need at least one committer as a maintainer (see https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#quick-start-to-adding-a-package). |
|
I feel like most criteria is check, im not so sure on the third point "How realistic is it that it will be used by other people?" but i havent heard of any other service that does the same and that is already present on nixpkgs, so i guess this adds something new? what are your toughs? |
pkgs/by-name/go/gologin/package.nix
Outdated
| meta = { | ||
| description = "Anti-detect browser for managing multiple accounts on a single device."; | ||
| homepage = "https://gologin.com/"; | ||
| mainProgram = pname; |
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 = pname; | |
| mainProgram = "gologin"; |
I think it's preferred to set the mainProgram separately.
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.
Yeah you are right
Always prefer using a hardcoded string (don't use pname, for example).
I think this applies to this package? |
This comment was marked as outdated.
This comment was marked as outdated.
You are right to check on that point. |
|
They had 30 updates starting from feb 2025, indeed its fast paced about 2.5 updates per month. |
|
It's not about how frequent the updates are, but rather how security essential the packages is. It applies to all browser now, but I'm not sure if this applies to this package as it differs a bit from the "main" browsers. It's probably good to ask someone with more expertise on this 👍 |
|
Well then, does that mean we should wait for someone to come and review or i should ping chrome maintainers? |
| version = "4.0.2"; | ||
|
|
||
| srcTar = fetchzip { | ||
| url = "https://dl.gologin.com/gologin.tar"; |
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 think this is going to work as the download link isn't version fixed. The hash would break when they update it.
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.
INDEED this is a issue here, they also dont provide any way to download older versions...
I dont see how to solve this, maybe its better to leave this outside nixpkgs
You can ask in the Matrix chat or maybe ask the security team. Or just wait for another reviewer. |
If the previous issue on the versions doesnt have a solution its not worth pursuing someone to help with this |
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.