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

Installers for Windows and macOS #3441

Merged
merged 3 commits into from
Mar 5, 2022
Merged

Installers for Windows and macOS #3441

merged 3 commits into from
Mar 5, 2022

Conversation

zah
Copy link
Member

@zah zah commented Feb 27, 2022

There are still some differences in the user experience between Linux and these new installers. These will be harmonized after introducing a config file support in a follow up PR.

sudo dscl . -create /Users/nimbus RealName "Nimbus"

sudo dscl . -create /Users/nimbus UniqueID "1012"
sudo dscl . -create /Users/nimbus PrimaryGroupID 80
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to hardcode user and group IDs?

Copy link
Member Author

@zah zah Mar 5, 2022

Choose a reason for hiding this comment

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

Will revisit this in follow-up PRs.

@@ -471,6 +471,14 @@ clean-gnosis-chain:
### Other
###

nimbus-msi: | nimbus_beacon_node
"$(WIX)/bin/candle" -ext WixUIExtension -ext WixUtilExtension installer/windows/*.wxs -o installer/windows/obj/
"$(WIX)/bin/light" -ext WixUIExtension -ext WixUtilExtension -cultures:"en-us;en-uk;neutral" installer/windows/obj/*.wixobj -out build/NimbusBeaconNode.msi
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is "WIX" coming from? The environment?

You can do a Wix binary access check, like in the various sanity checks already in place.

Copy link
Member Author

Choose a reason for hiding this comment

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

WIX is an environment variable created by the WIX installer. Since our installer uses only relatively basic and old WIX functionalities, it should work with any WIX version.

@github-actions
Copy link

github-actions bot commented Feb 28, 2022

Unit Test Results

     12 files  ±0     821 suites  ±0   34m 58s ⏱️ + 1m 14s
1 672 tests ±0  1 625 ✔️ ±0    47 💤 ±0  0 ±0 
9 759 runs  ±0  9 655 ✔️ ±0  104 💤 ±0  0 ±0 

Results for commit 3795d57. ± Comparison against base commit cdeae90.

♻️ This comment has been updated with latest results.

@@ -29,6 +29,64 @@ from
import
TopicParams, validateParameters, init

when defined(windows):
Copy link
Member

Choose a reason for hiding this comment

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

can we move the special windows sauce to a separate module? nimbus_beacon_node is already quite hard to follow because it does a lot of things, and slowly we've been refactoring it to become more of a delegator than a module for actual implementations (see deposits, trusted node sync etc) - the windows support would ideally reach a similar level of isolation

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, no, because the service code needs to refer and call functions from nimbus_beacon_node (not the other way around). You can work-around this with function pointers perhaps, but it would arguably make the code more convoluted and harder to follow. A more reasonable alternative might be to introduce a new module called entry_point or something along those that wraps nimbus_beacon_node and sets up the service when necessary.

@@ -184,6 +181,11 @@ type
defaultValue: BNStartUpCmd.noCommand }: BNStartUpCmd

of BNStartUpCmd.noCommand:
runAsServiceFlag* {.
Copy link
Member

Choose a reason for hiding this comment

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

this flag would ideally not be exposed on linux - even on windows, perhaps it should be hidden given that it shouldn't be used from the command line

Copy link
Member Author

Choose a reason for hiding this comment

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

Hidden on non-windows targets through conditional compilation. At the moment, it's not easy to completely remove the option, because the compile-time reflection of Nim doesn't provide a good way to resolve when statements during the examination of the configuration record type. Furthermore, accepting the option on non-windows targets may still be beneficial because it simplifies the portability of Nimbus-launching scripts.

@@ -0,0 +1,955 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

this project file looks massive and unmaintainable without a full mac development environment - it seems like there should exist some more automation-friendly way to do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

We already use the most automation-friendly way to create the packages which is the following non-Apple software created specifically for this purpose:

http://s.sudre.free.fr/Software/Packages/about.html

Nevertheless, the workflow still requires the use of XCode for editing the package properties.

@arnetheduck
Copy link
Member

These will be harmonized

I'm not convinced this is a good idea - this will be a lowest common denominator solution where we try to fit a square peg in a round, triangular and oval hole respectively - integration is one area where you end up with a sub-par experience for some of the platforms if you try to do this, ie in systemd, the service file is the configuration file (even if you have the possibility to work against the way the system was designed, and use a config file anyway - in windows, this is supposed to be the registry, and so on)

@arnetheduck
Copy link
Member

arnetheduck commented Feb 28, 2022

where will nodes created by these installers write their data? since the database is expected to grow well in excess of 50gb, writing it to hidden locations, C: in general (which holds OS and tends to be small, and that may render the system unusuable when strained for space), or worse, the roaming profile folder, would be really bad - on unixes, it's somewhat easier in that /var/lib is the accepted folder - it's scoffed upon to write 50gb to $HOME/.config, and .cache is would be wrong because the database carries security-sensitive information (the head root for example which determines what constitutes the valid block) as well as the slashing db which should not be removed.

@zah
Copy link
Member Author

zah commented Mar 5, 2022

where will nodes created by these installers write their data? since the database is expected to grow well in excess of 50gb, writing it to hidden locations, C: in general (which holds OS and tends to be small, and that may render the system unusuable when strained for space), or worse, the roaming profile folder, would be really bad - on unixes, it's somewhat easier in that /var/lib is the accepted folder - it's scoffed upon to write 50gb to $HOME/.config, and .cache is would be wrong because the database carries security-sensitive information (the head root for example which determines what constitutes the valid block) as well as the slashing db which should not be removed.

I think we can follow the footsteps of PostgreSQL, MSSQL and MySQL - all of them write their data files in a sub-folder of their installation directory in "Program Files".

@zah
Copy link
Member Author

zah commented Mar 5, 2022

I'm not convinced this is a good idea - this will be a lowest common denominator solution where we try to fit a square peg in a round, triangular and oval hole respectively - integration is one area where you end up with a sub-par experience for some of the platforms if you try to do this, ie in systemd, the service file is the configuration file (even if you have the possibility to work against the way the system was designed, and use a config file anyway - in windows, this is supposed to be the registry, and so on)

I don't think there is a strong tradition to consider the systemd unit file as a configuration file. People usually prefer to have an easy way to launch the software manually and the less options are required, the better. Default location for the configuration file or at least a way to say some-program --config=/etc/some-program.conf is much closer to what most users expect from mature software.

I'm a bit torn regarding the use of the Windows registry. The downside would be that our documentation would need to have separate sections for Windows configuration and editing the registry may be perceived as more dangerous by some users. The upside is that this indeed follows the tradition of the platform and there are some windows administration features that become more readily available if your configuration is in the registry. Perhaps we should support using the registry as an opt-in feature.

In any case, we don't want most users to mess with configuration files or the registry in the long run, because we'll expose all configuration through the command-line(nimbus config set web3-url ...) and through GUIs.

@zah zah merged commit 3795d57 into unstable Mar 5, 2022
@zah zah deleted the run-as-service branch March 5, 2022 14:39
@arnetheduck
Copy link
Member

all of them write their data files in a sub-folder of their installation directory in "Program Files".

Isn't that for pre-xp compatibility? afair "program files" has permission restrictions for non-admin users on modern windowses - shouldn't it be something like https://docs.microsoft.com/en-us/windows-hardware/customize/desktop/unattend/microsoft-windows-shell-setup-folderlocations-programdata?

@arnetheduck
Copy link
Member

I don't think there is a strong tradition to consider the systemd unit file as a configuration file.

well, it's what the systemd manual recommends as well as random people on stackoverflow. environment files, separate config files and so on are, I believe, used to ease migration from sysv init and the like, and for .. I guess people that grew up before systemd and didn't want to bother changing?

Perhaps we should support using the registry as an opt-in feature.

probably reasonable, specially in windows-service-mode. that said, this is more broadly part of a more general problem: providing installers and platform integration is maintenance-intensive and requires care, knowledge and attention for each platform separately, or the integration work will feel like git for windows, which arguably .. sucks - it's hard to uphold quality over time with such features, and they steal attention from other work.

@zah
Copy link
Member Author

zah commented Mar 7, 2022

Isn't that for pre-xp compatibility? afair "program files" has permission restrictions for non-admin users on modern windowses - shouldn't it be something like https://docs.microsoft.com/en-us/windows-hardware/customize/desktop/unattend/microsoft-windows-shell-setup-folderlocations-programdata?

The files in the Program Files folder are protected from accidental user actions by default, but this doesn't mean that a service cannot manipulate them. The aforementioned databases uses their own "service account" which is given the right permissions to write to the data folder:

https://docs.microsoft.com/en-us/windows/security/identity-protection/access-control/service-accounts

LPCSTR* = cstring
LPSTR* = cstring

SERVICE_STATUS* {.final, pure.} = object
Copy link
Member

Choose a reason for hiding this comment

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

well, stuff like this surely does not need to pollute nimbus_beacon_node

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants