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

dae: add support for geolocation databases #244129

Merged
merged 1 commit into from
Jul 19, 2023
Merged

dae: add support for geolocation databases #244129

merged 1 commit into from
Jul 19, 2023

Conversation

1sixth
Copy link
Contributor

@1sixth 1sixth commented Jul 18, 2023

Description of changes

also installs a systemd service

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from oluceps July 18, 2023 08:06
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jul 18, 2023
@oluceps
Copy link
Member

oluceps commented Jul 18, 2023

I recommend adding a NixOS module as an alternative or making some modifications to the upstream unit files. Here are the reasons:

dae relies on a geolocation database at runtime. If it cannot be found in hardcoded absoluted path or xdg data dir (May not exist?) it fails to start. This may be hard to handle if install the service directly. See https://github.com/daeuniverse/dae/blob/242bc19e4a8e4d052cb45ab37b76dc556ce692fe/common/assets/assets.go#L71 and #81138.

@1sixth
Copy link
Contributor Author

1sixth commented Jul 19, 2023

I've updated the code to let dae use geolocation databases. It's mostly copied from v2ray/default.nix, but I didn't add support to override assets because of the way dae searches assets:

  1. If DAE_LOCATION_ASSET is set, dae expects them to be in that directory, it won't search sub-directories.
  2. If XDG_DATA_DIRS is set, dae expects them to be in $XDG_DATA_DIRS/dae.

I can't really find a nice way to make dae use other databases like sing-geoip.

@1sixth 1sixth changed the title dae: install systemd service dae: add support for geolocation databases Jul 19, 2023
@oluceps oluceps requested a review from NickCao July 19, 2023 05:07
@oluceps
Copy link
Member

oluceps commented Jul 19, 2023

I can't really find a nice way to make dae use other databases like sing-geoip.

sing-geoip isn't compatible with dae. It doesn't matter.

Considering nixpkgs is also for non-NixOS systems, is the unit file work in that case?

Maybe adding a nixos module instead is a better choice.

@1sixth
Copy link
Contributor Author

1sixth commented Jul 19, 2023

is the unit file work in that case?

probably not, see https://discourse.nixos.org/t/how-to-start-systemd-services-provided-by-packages-installed-through-nix-on-an-alien-distro/23056

Maybe adding a nixos module instead is a better choice.

I agree that a nixos module is better. But it doesn't conflict with shipping a systemd service. For example, the v2ray module and the sing-box module both use the systemd service from upstream:

systemd.packages = [ cfg.package ];

systemd.packages = [ cfg.package ];

installPhase = ''
runHook preInstall
install -Dm555 "$GOPATH"/bin/dae $out/bin/dae
install -Dm444 install/dae.service $out/lib/systemd/system/dae.service
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps both the installation of the service and the steps in postFixup can be moved to the postInstall phase to avoid modifying the installPhase. Apart from that, it looks good to me.

@oluceps
Copy link
Member

oluceps commented Jul 19, 2023

Result of nixpkgs-review pr 244129 run on x86_64-linux 1

1 package built:
  • dae

@oluceps oluceps added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jul 19, 2023
@ofborg ofborg bot requested a review from oluceps July 19, 2023 07:25
};

postInstall = ''
install -Dm555 "$GOPATH"/bin/dae $out/bin/dae
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this line redundant? (the binary should already be installed in the installPhase)

also installs a systemd service
@NickCao NickCao merged commit 1899ff0 into NixOS:master Jul 19, 2023
@1sixth 1sixth deleted the dae branch July 19, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants