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

Packager: Rename Daemon + Fix for Release mode #10883

Merged

Conversation

kiminuo
Copy link
Collaborator

@kiminuo kiminuo commented Jun 13, 2023

Addresses

Lucas Ontivero 3 days ago I think the next release is not shipping the daemon. It will be there but the packager doesn't remaned it as wassabeed as i think it should

from https://github.com/orgs/zkSNACKs/projects/27/views/1

Testing

# Package for all platforms
cd C:\WasabiWallet\WalletWasabi\WalletWasabi.Packager
dotnet restore && dotnet build && dotnet run # or: dotnet restore && dotnet build && dotnet run -- --onlybinaries

# Result is placed here
cd C:\WalletWasabi\WalletWasabi.Fluent.Desktop\bin\dist\win7-x64
wassabeed.exe

I'm not exactly why this leads to starting the Daemon process but not waiting for user input (pressing CTRL+C). I tried to add Console.Read() command but it did not help. Investigating. Maybe I'm doing something trivially wrong. Fixed by bc2a139.

Help is welcome.

edit: Testing the same on macOS works OK for me. So it's only on Win11 that wassabeed.exe does not wait for console input (on my machine at least).

edit: edit: Fixed.

@kiminuo kiminuo changed the title Packager: Rename Daemon Packager: Rename Daemon + Fix for Release mode Jun 13, 2023
@kiminuo kiminuo marked this pull request as ready for review June 13, 2023 11:20
@kiminuo kiminuo requested a review from lontivero June 13, 2023 12:00
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

Tested running wassabeed.exe with the command line.
LGTM

Copy link
Collaborator

@lontivero lontivero left a comment

Choose a reason for hiding this comment

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

It looks very good for me, in fact, I like it. However, it is @molnard the "owner" and I would would prefer he approve it.
I didn't find anything wrong btw.

<ItemGroup>
<ProjectReference Include="..\WalletWasabi\WalletWasabi.csproj" />
</ItemGroup>
<ItemGroup>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

spaces -> tabs

</PropertyGroup>

<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|AnyCPU'">
<OutputType>WinExe</OutputType>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

WinExe is only for UI applications (eg WinForms or WPF). That's why in the Release mode the program did not wait for any console input.

@lontivero
Copy link
Collaborator

@adamPetho can you please test it?

@@ -25,6 +25,8 @@ namespace WalletWasabi.Packager;
public static class Program
{
public const string PfxPath = "C:\\digicert.pfx";

public const string DaemonExecutableName = Constants.DaemonExecutableName;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done for consistency with the line below.

@@ -25,6 +25,8 @@ namespace WalletWasabi.Packager;
public static class Program
{
public const string PfxPath = "C:\\digicert.pfx";

public const string DaemonExecutableName = Constants.DaemonExecutableName;
public const string ExecutableName = Constants.ExecutableName;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it would be better to have:

Suggested change
public const string ExecutableName = Constants.ExecutableName;
public const string UiExecutableName = Constants.UiExecutableName;

We can change it if you like.

Copy link
Collaborator

@adamPetho adamPetho left a comment

Choose a reason for hiding this comment

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

tACK on Win11
Packager run with --onlybinaries attribute, wassabeed.exe (Daemon) worked just fine.

@adamPetho adamPetho requested a review from molnard June 14, 2023 15:50
@lontivero
Copy link
Collaborator

Ok, it looks very safe to me.

@lontivero lontivero merged commit bae6e3b into zkSNACKs:master Jun 14, 2023
9 checks passed
@kiminuo kiminuo deleted the feature/2023-06-13-Packager-n-Daemon branch June 14, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants