build: add RPM for gateway and agent #1179

allan2 · 2025-01-06T23:10:56Z

This commit adds RPM packages for Gateway and Agent to the release assets.

tlk.ps1 -TlkVerb package -Platform linux now generates both a deb and an rpm for the specified product.

The rpm is generated with fpm, a Linux packaging tool.
In CI, no cache is used for the fpm dependency since the cache would not persist across release jobs.

The RPM includes all the assets of the corresponding Debian package, including the changelog, copyright, maintainer scripts, and webapp/libxmf.so for Gateway.

The changelog is copied directly from Debian. A separate PR will be made to convert it to an RPM-friendly changelog format.

Tested with RHEL 9 (glibc 2.34).

github-actions · 2025-01-06T23:11:19Z

Let maintainers know that an action is required on their side

Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)
Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release
Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)
Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries

This commit adds a package like `devolutions-gateway-2024.3.6.0_x86_64.rpm` to release assets. The RPM is generated with fpm, a Linux packaging tool. No cache is used for the fpm dependency since the cache would not persist across release jobs. Installation and execution was tested on RHEL 9 (glibc 2.34).

thenextman · 2025-01-06T23:39:56Z

.github/workflows/ci.yml

@@ -727,6 +727,10 @@ jobs:
        if: matrix.os == 'windows'
        uses: microsoft/setup-msbuild@v2

+      - name: Install fpm


Is it in the right place? This is the devolutions-agent job, but the PR references Gateway

Fixed in b69f0c5.

thenextman · 2025-01-06T23:41:03Z

.github/workflows/ci.yml

@@ -727,6 +727,10 @@ jobs:
        if: matrix.os == 'windows'
        uses: microsoft/setup-msbuild@v2

+      - name: Install fpm
+        if: runner.os == 'Linux'


minor nit: maybe use matrix.os; it doesn't really matter here but in a cross compilation environment we could be building on a Linux runner but targeting another OS. The matrix.os holds the OS we are targetting.

Fixed here and here.

This adds an RPM package for agent. Tested with RHEL 9.

allan2 · 2025-01-07T12:59:06Z

This PR is not ready for review yet. I'm still adding things to bring it to parity with the Debian package.

thenextman · 2025-01-07T13:58:31Z

This PR is not ready for review yet. I'm still adding things to bring it to parity with the Debian package.

As a hint, can be good to make the PR a draft in this case. Because I think the repo automatically adds arch-maintainers as reviewer, which generates an email prompting for review :)

Stripping first ensures the packaged binary is identical for both deb and rpm. [skip ci]

CBenoit · 2025-01-07T18:02:12Z

ci/tlk.ps1

+        # Strip first so the included binary is identical for deb and rpm
+        & 'strip' $Executable | Out-Host


question: Maybe we should strip somewhere else? There is an option in Cargo.toml to enable automatic stripping in profiles: https://doc.rust-lang.org/cargo/reference/profiles.html#strip
Configuring this appropriately for the production profile, and removing manual calls to strip would be superior I think.

I'd rather strip here.

When working on packaging, I often use debug builds for the faster compile time.
Keeping symbols in the release build can be useful for troubleshooting.

If you still prefer to strip in cargo, I can change it.

Keeping symbols in the release build can be useful for troubleshooting.

Are you thinking about troubleshooting using a local "release build" on your dev machine?

For binaries that we ship, separating the debug symbols from the binaries and only delivering the binaries without the symbols is still superior (splitting the symbols from the binary). We are doing that at least for Windows builds, not yet for Linux and macOS builds. There is also an option in Cargo that may be used for splitting debug symbols: https://doc.rust-lang.org/cargo/reference/profiles.html#split-debuginfo
But here we are just stripping unconditionally in the script, so I’ll assume you are referring to testing with a local build that is not going through the script.

So for local builds, don’t worry. I’m not asking that we modify the release profile, but the production profile.
We defined additional profiles in the top-level Cargo.toml:

devolutions-gateway/Cargo.toml

Lines 23 to 29 in 0ed70d2

[profile.profiling]

inherits = "release"

debug = 1

[profile.production]

inherits = "release"

lto = true

The production profile is only used when we produce final artifacts that we want to ship, in the CI, and this is a parameter of the script itself:

devolutions-gateway/.github/workflows/ci.yml

Lines 117 to 127 in 0ed70d2

- name: Configure rust profile

id: rust-profile

shell: pwsh

run: |

$CargoProfile = "release"

if ("${{ github.ref }}" -Eq "refs/heads/master") {

echo "::notice::Building production profile"

$CargoProfile = "production"

}

echo "rust-profile=$CargoProfile" >> $Env:GITHUB_OUTPUT

devolutions-gateway/.github/workflows/ci.yml

Line 270 in 0ed70d2

./ci/tlk.ps1 build -Product jetsocat -Platform ${{ matrix.os }} -Architecture ${{ matrix.arch }} -CargoProfile ${{ needs.preflight.outputs.rust-profile }}

I think it’s a more declarative and flexible way of handling this problem. What do you think? I’m open to hear otherwise if I’m missing some drawback.

Gotcha. I was not aware of the production profile. How's this?

[profile.production] inherits = "release" lto = true strip = "symbols"

And I will keep strip disabled on dpkg-buildpackage so that the deb and rpm ship an identical binary, regardless of whether it is debug/release/stripped/not stripped.

My proposal is in 4b4e413.

I think this is good. Can you check if you get the same binary size with this option enabled without running strip, and this optional disabled but running strip?

ls -l --block-size=1 (output edited)

36321608 release 28875432 release-strip 28879528 release-cargostrip 28879528 release-cargostrip-strip (no change) 29466552 release-lto 25467424 release-lto-strip 25451040 release-lto-cargostrip (production profile) 25451040 release-lto-cargostrip-strip (no change)

For thin LTO, manual strip was 1.4% smaller than strip = "symbols".
For fat LTO, strip = "symbols" was 0.06% smaller than manual strip.

So for the production profile, strip = "symbols" won out by the tiniest of margins.

Now please don't ask me to check -O2 and -O3 :)

Oh… I’m sorry I didn’t mean to ask you to check with various optimization profiles 😂
Just wanted to see if strip = "symbols" was indeed identical to calling strip manually for the same profile, because there are other possible values for this option. Anyway, it’s likely the right value :)
Thank you for the other numbers too!

ci/tlk.ps1

[skip ci]

thenextman · 2025-01-07T19:59:46Z

ci/tlk.ps1

@@ -674,6 +676,7 @@ class TlkRecipe
                } else {
                    throw ("Specify DGATEWAY_WEBCLIENT_PATH environment variable")
                }
+                $DGatewayWebPlayer = "$($this.SourcePath)/webapp/player"


disclaimer: I'm probably just not understanding (I'm only reading the PR without full context)

The player comes direct from the source tree (i.e. it's not something compiled)?

Just reading this implies that there should be something in ./webapp/player, but I don't see anything in that path when I browse the repo?

Probably a good catch. @irvingoujAtDevolution modified that recently, maybe this branch wasn’t up to date?

ci: build player artifacts on demand #1169

refactor(player): player project, introduce typescript and vite #1163

Both the client (root of webapp) and the player (root of webapp/player-project) are Angular frontends which have to be built.

In ci.yml, the client is built here and the player is built here.

Environment variables are then set here, which are read by tlk.ps1 to copy the static files into /usr/share/webapp.

Previously, there was only DGATEWAY_WEBCLIENT_PATH but I just added DGATEWAY_WEBPLAYER_PATH in 6a6442a. Since I am just passing in a prebuilt client and player, this allows for specifying the path to the player, instead of requiring it to be present in the webapp/player directory (the source of your current confusion and my earlier confusion).

Excellent, thanks!

thenextman · 2025-01-07T20:01:30Z

I queried one more thing that I'm probably just missing context for.

Wouldn't it be good if we could build the .deb using fpm as well? As it stands now, if I want to modify the packaging, I need to understand both dpkg and fpm. I understood the promise of fpm was that we could build different package types and have all that inner knowledge abstracted away.

Anyway, just a thought; I'm not suggesting making it part of this PR. But it would be a nice additional improvement in future!

This commit adds DGATEWAY_WEBPLAYER_PATH to accompany the existing DGATEWAY_WEBCLIENT_PATH. When packaging Gateway by running tlk.ps1, the DGATEWAY_WEBCLIENT_PATH must be specified. Before this commit, the player must be present in repo_root/webapp/player. After this commit, the DGATEWAY_WEBPLAYER_PATH must be specified. This allows for packaging with a pre-built player without copying it into the hardcoded path.

allan2 · 2025-01-07T21:49:57Z

Two more things are unresolved:

dependencies are missing in rpm. I'm currently working on this.
WindowsManager/Program.cs has a check for the web client path but not for the web player path. Should I add it in?

thenextman · 2025-01-08T00:49:12Z

Two more things are unresolved:

dependencies are missing in rpm. I'm currently working on this.

WindowsManager/Program.cs has a check for the web client path but not for the web player path. Should I add it in?

Yes, if you can add the environment variable and usage into the managed installer, that would be great. Can just follow what is there for the web client currently. Thank you! 💯

thenextman · 2025-01-08T01:16:13Z

One more thing if you wouldn't mind @allan2, can you update the file spec here

devolutions-gateway/package/WindowsManaged/Program.cs

Line 314 in 0ed70d2

new("player", new Files(@"..\..\webapp\player\*.*"))

to use the property you added instead the hardcoded path? Can be a followup PR if that suits you better.

allan2 requested review from a team as code owners January 6, 2025 23:10

allan2 force-pushed the rpm branch from 1f6944f to 920979e Compare January 6, 2025 23:13

thenextman reviewed Jan 6, 2025

View reviewed changes

allan2 added 2 commits January 6, 2025 19:44

fix(ci): add missing dependency for dgw RPM

b69f0c5

feat(agent): add RPM package

0abc7f4

This adds an RPM package for agent. Tested with RHEL 9.

allan2 changed the title ~~feat(dgw): add RPM package~~ feat: add RPM for gateway and agent Jan 7, 2025

allan2 changed the title ~~feat: add RPM for gateway and agent~~ build: add RPM for gateway and agent Jan 7, 2025

Add /usr/share assets

83e0926

allan2 marked this pull request as draft January 7, 2025 14:45

allan2 added 2 commits January 7, 2025 09:49

Merge latest master

212da9e

Add maintainer scripts to RPM

b45dda9

allan2 marked this pull request as ready for review January 7, 2025 15:24

Strip binary before packaging

54cee71

Stripping first ensures the packaged binary is identical for both deb and rpm. [skip ci]

CBenoit reviewed Jan 7, 2025

View reviewed changes

ci/tlk.ps1 Outdated Show resolved Hide resolved

CBenoit reviewed Jan 7, 2025

View reviewed changes

ci/tlk.ps1 Outdated Show resolved Hide resolved

allan2 added 2 commits January 7, 2025 13:44

Add explanation for disabling stripping

2a10c88

[skip ci]

Style fixes

926709e

[skip ci]

thenextman reviewed Jan 7, 2025

View reviewed changes

Strip production profile symbols [skip ci]

4b4e413

Add check for web player path

d05402d

Use player path field declaration in C#

ced1fd6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add RPM for gateway and agent #1179

build: add RPM for gateway and agent #1179

allan2 commented Jan 6, 2025 •

edited

Loading

github-actions bot commented Jan 6, 2025

thenextman Jan 6, 2025

allan2 Jan 7, 2025

thenextman Jan 6, 2025

allan2 Jan 7, 2025

allan2 commented Jan 7, 2025

thenextman commented Jan 7, 2025

CBenoit Jan 7, 2025 •

edited

Loading

allan2 Jan 7, 2025

CBenoit Jan 7, 2025

allan2 Jan 7, 2025

allan2 Jan 7, 2025

CBenoit Jan 7, 2025

allan2 Jan 7, 2025 •

edited

Loading

CBenoit Jan 7, 2025 •

edited

Loading

thenextman Jan 7, 2025

CBenoit Jan 7, 2025

allan2 Jan 7, 2025

thenextman Jan 8, 2025

thenextman commented Jan 7, 2025 •

edited

Loading

allan2 commented Jan 7, 2025 •

edited

Loading

thenextman commented Jan 8, 2025

thenextman commented Jan 8, 2025

		# Strip first so the included binary is identical for deb and rpm
		& 'strip' $Executable \| Out-Host

	[profile.profiling]
	inherits = "release"
	debug = 1

	[profile.production]
	inherits = "release"
	lto = true

	- name: Configure rust profile
	id: rust-profile
	shell: pwsh
	run: \|
	$CargoProfile = "release"
	if ("${{ github.ref }}" -Eq "refs/heads/master") {
	echo "::notice::Building production profile"
	$CargoProfile = "production"
	}

	echo "rust-profile=$CargoProfile" >> $Env:GITHUB_OUTPUT

build: add RPM for gateway and agent #1179

Are you sure you want to change the base?

build: add RPM for gateway and agent #1179

Conversation

allan2 commented Jan 6, 2025 • edited Loading

github-actions bot commented Jan 6, 2025

Let maintainers know that an action is required on their side

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allan2 commented Jan 7, 2025

thenextman commented Jan 7, 2025

CBenoit Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allan2 Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

CBenoit Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thenextman commented Jan 7, 2025 • edited Loading

allan2 commented Jan 7, 2025 • edited Loading

thenextman commented Jan 8, 2025

thenextman commented Jan 8, 2025

allan2 commented Jan 6, 2025 •

edited

Loading

CBenoit Jan 7, 2025 •

edited

Loading

allan2 Jan 7, 2025 •

edited

Loading

CBenoit Jan 7, 2025 •

edited

Loading

thenextman commented Jan 7, 2025 •

edited

Loading

allan2 commented Jan 7, 2025 •

edited

Loading