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

scripts,GitHubCI,Backend: add .NET6 snap #207

Closed
wants to merge 1 commit into from
Closed

scripts,GitHubCI,Backend: add .NET6 snap #207

wants to merge 1 commit into from

Conversation

webwarrior-ws
Copy link
Contributor

@webwarrior-ws webwarrior-ws commented Feb 17, 2023

Use .NET6 to build snap package.
Publish Frontend.Console as single executable in in snap_build
script.

Modified launch script to point to gwallet executable generated
with new settings.

Set InvariantGlobalization to true in Backend and
Frontend.Console projects to fix error when launching
gwallet installed by snap.
Error in question was:
Process terminated. Couldn't find a valid ICU package installed
on the system. Please install libicu using your package manager
and try again. Alternatively you can set the configuration flag
System.Globalization.Invariant to true if you want to run with
no globalization support.

Set correct configPath when GWallet is in a snap package.

Build mono snap package alongside .net6. Also upload mono snap
package as artifact and test it.
It is needed because the frontend branch can't switch yet to
dotnet-based snap.

When building snap package with .NET6, only include single-file
executable in package.
Added "publish" command to make.fsx and Makefile.

Superseded by #297

scripts/snap_build.sh Outdated Show resolved Hide resolved
@knocte
Copy link
Member

knocte commented Feb 20, 2023

Set InvariantGlobalization to true in Backend and
Frontend.Console projects to fix error when launching
gwallet installed by snap.

Fix error? What error? Please dump the whole error in the commit message, no need to be secretive about it.

One thing I noticed though is when making snap package with dotnet,
all build artifacts end up in a package, even though we don't need them
as everything is packed into 1 executable

This needs to be fixed indeed, otherwise the snap package is unnecessarily big.

And I'm not sure what is the best way to remove them.
make install both builds in release mode and copies needed files.
Maybe delete unneeded files in destination dir (under ./staging) after make install?

No. As I suggested in the review, first change make.fsx to call dotnet publish (the developer should not need to know the way to call dotnet publish, the point of using Makefiles is to have make do everything). After you've done that, change the dotnet publish call to not use the -o flag (to keep the executable in its proper publish folder, which signals in a good way that it's architecture-specific); and after that, change the 'install' target to make it check first if the publish folder exists (and if it does, copy just the single executable instead of the other files).

Copy link
Contributor

@aarani aarani left a comment

Choose a reason for hiding this comment

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

LGTM

scripts/make.fsx Outdated Show resolved Hide resolved
scripts/make.fsx Outdated Show resolved Hide resolved
scripts/make.fsx Outdated Show resolved Hide resolved
scripts/make.fsx Outdated Show resolved Hide resolved
@knocte
Copy link
Member

knocte commented Feb 20, 2023

When addressing my last review, while you're at it, please squash into 1 commit, there's no reason to separate this into different commits, unless I'm missing something.

.github/workflows/CI.yml Outdated Show resolved Hide resolved
scripts/make.fsx Outdated Show resolved Hide resolved
scripts/make.fsx Outdated Show resolved Hide resolved
@knocte
Copy link
Member

knocte commented Feb 22, 2023

@webwarrior-ws 2 things:

  • When I give feedback on your PR; for each comment of mine that you address please reply with "Done" and click on "Resolve" button after pushing a commit that addresses the comment
  • Nit: I didn't ask you to explain every single detail of what you're doing in the commit message body; I only ask you to add things to the commit message body when they are not self-explanatory (e.g. renaming snap_pkg to have _dotnet suffix because you also add another snap_pkg_mono suffix is self-explanatory; do not waste time explaining it).

@knocte
Copy link
Member

knocte commented Feb 22, 2023

2 things:

A 3rd thing actually: given that we're maintaining the mono snap package instead of changing it to always use dotnet, the title of commit msg (and PR title) needs to be updated.

scripts/make.fsx Outdated Show resolved Hide resolved
@webwarrior-ws webwarrior-ws changed the title Use .NET6 to build snap package Build 2 snap packages: using mono and using .NET6 Feb 22, 2023
scripts/make.fsx Outdated Show resolved Hide resolved
scripts/make.fsx Outdated Show resolved Hide resolved
.github/workflows/CI.yml Outdated Show resolved Hide resolved
@knocte
Copy link
Member

knocte commented Feb 23, 2023

Last thing to fix before I merge this is cosmetic:

  • If the PR has only 1 commit, what's the reason for the PR title and PR description to not match exactly with commit message title and commit message body? There's no reason for them to be different.
  • WRT the commit msg title, what in the world does FE mean? I only used the abbreviation FE once myself, but it was used along the suffix .Console, that is, FE.Console, because the areas to put in that part of the commit message have to map to real projects. There's no "FE" or "Frontend" project, so if you use this in the title, one might think that this applies to all frontends, which is not the case (because this commit is not modifying Android or Gtk frontends, for example). I think a better area for this commit should start with scripts,GitHubCI,Backend: because the minimal change in FE.Console.fsproj doesn't really matter, the area is already too crowded. Also "using mono and .NET6" is not a good title because master branch is already using mono, so you don't need to mention it. A better title would simply be scripts,GitHubCI,Backend: add .NET6 snap.

Use .NET6 to build snap package.
Publish Frontend.Console as single executable in in snap_build
script.

Modified launch script to point to gwallet executable generated
with new settings.

Set InvariantGlobalization to true in Backend and
Frontend.Console projects to fix error when launching
gwallet installed by snap.
Error in question was:
Process terminated. Couldn't find a valid ICU package installed
on the system. Please install libicu using your package manager
and try again. Alternatively you can set the configuration flag
System.Globalization.Invariant to true if you want to run with
no globalization support.

Set correct configPath when GWallet is in a snap package.

Build mono snap package alongside .net6. Also upload mono snap
package as artifact and test it.
It is needed because the frontend branch can't switch yet to
dotnet-based snap.

When building snap package with .NET6, only include single-file
executable in package.
Added "publish" command to make.fsx and Makefile.
@webwarrior-ws webwarrior-ws changed the title Build 2 snap packages: using mono and using .NET6 scripts,GitHubCI,Backend: add .NET6 snap Feb 23, 2023
@webwarrior-ws
Copy link
Contributor Author

Updated commit message and PR title/description

@knocte knocte force-pushed the master branch 2 times, most recently from 1a3be72 to 265b103 Compare October 16, 2023 05:05
@knocte knocte force-pushed the master branch 2 times, most recently from b841302 to ccb1641 Compare May 8, 2024 09:18
@webwarrior-ws webwarrior-ws closed this by deleting the head repository May 23, 2024
@webwarrior-ws
Copy link
Contributor Author

Superseded by #297

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.

3 participants