Skip to content

Conversation

@dcelasun
Copy link
Contributor

Hi! I've built a package for Arch Linux to make my own life easier and wanted to mention it here in case it's useful. Feel free to close if not :)

@Athou
Copy link
Owner

Athou commented Feb 21, 2025

Sure, thanks!

Is this the install script? If so, I'm not very familiar with Arch but I have some remarks:

  • I don't think npm is needed as a dependency, maven will download the required version of node and npm in ./commafeed-client/node during build, regardless of the installed npm version on the system.
  • I don't think you need jdk21-openjdk if you have jdk21-graalvm-bin, you can probably point both JAVA_HOME and GRAALVM_HOME to /usr/lib/jvm/java-21-graalvm/.
  • you probably shouldn't copy the application.properties file from the sources, this file contains default values that were not supposed to be exposed to end users, CommaFeed might even break if some of those properties are changed.

dcelasun added a commit to dcelasun/pkgbuilds that referenced this pull request Feb 21, 2025
@dcelasun
Copy link
Contributor Author

I don't think npm is needed as a dependency, maven will download the required version of node

Good point, removed.

I don't think you need jdk21-openjdk if you have jdk21-graalvm-bin

Gave it a try and you were right! Removed jdk21-openjdk as well.

you probably shouldn't copy the application.properties

Hmm, it's best practice (in Arch packages) to provide a default config file that the user can then customize. If there are changes to the default file in future versions, pacman (Arch's package manager) doesn't overwrite the existing one and instead notifies you so you can manually review any upstream changes.

Of course, I'm happy to consider alternatives if you have something in mind.

@Athou
Copy link
Owner

Athou commented Feb 21, 2025

Hmm, it's best practice (in Arch packages) to provide a default config file that the user can then customize. If there are changes to the default file in future versions, pacman (Arch's package manager) doesn't overwrite the existing one and instead notifies you so you can manually review any upstream changes.

Of course, I'm happy to consider alternatives if you have something in mind.

Oh OK I understand.
I don't really have an alternative to suggest though. The properties that are end-user oriented and their default values are defined here. None of those properties are in commafeed-server/src/main/resources/application.properties :/

@dcelasun
Copy link
Contributor Author

None of those properties are in commafeed-server/src/main/resources/application.properties

Maybe we can add these to the top of that file? We can also split it into sections like "basic" and "advanced" and these would be in basic.

@Athou
Copy link
Owner

Athou commented Feb 21, 2025

The thing is commafeed-server/src/main/resources/application.properties is really only used internally to configure Quarkus. It could have been done programmatically, the fact that this file exists should not be relied upon by your script.

I don't really want to maintain an application.properties file that has the same content as CommaFeedConfiguration.java either. Ideally, to do what you want to do, an application.properties should be generated from CommaFeedConfiguration.java, but I don't think such a generator exists.

@dcelasun
Copy link
Contributor Author

dcelasun commented Feb 21, 2025

Makes sense. How about this? I'll open a PR for a sample application.properties file with user-facing configuration only, right next to commafeed.md. Then my package can just use that.

If you don't want to maintain that (which I understand) I can just do it in my package.

@Athou
Copy link
Owner

Athou commented Feb 22, 2025

Actually, writing a generator wasn't that bad, I wrote one in this branch. This is the output (renamed to .txt because GitHub doesn't let me upload properties files), does it work for you?
application.properties.txt

It will automatically be generated during build in commafeed-server/target/quarkus-generated-doc/application.properties

@dcelasun
Copy link
Contributor Author

That's perfect! Yes, it would work just fine. I'll update my package once you merge.

@Athou
Copy link
Owner

Athou commented Feb 22, 2025

It's been merged in master and will be part of the next release.

@dcelasun
Copy link
Contributor Author

Great, I'll push it with 5.7.0 then.

@Athou Athou merged commit 28a4bb4 into Athou:master Feb 23, 2025
17 checks passed
@Athou
Copy link
Owner

Athou commented Feb 23, 2025

I just released 5.6.1 that includes the generator 😄

@dcelasun
Copy link
Contributor Author

Thank you! I just pushed my update as well :)

By the way, would it be feasible to add a command to include all databases in the binary? Right now I'm generating one package per supported db, but it'd be great to unify them.

@Athou
Copy link
Owner

Athou commented Feb 23, 2025

That's not possible. Quarkus does a lot of work at build time to avoid having to do it at runtime. The database choice is made at build time, this is why there are 4 build profiles for the 4 supported databases.

@dcelasun
Copy link
Contributor Author

Yeah, I figured. No worries, it works just fine as is :)

@dcelasun dcelasun deleted the arch-package branch May 6, 2025 21:13
archlinux-github pushed a commit to archlinux/aur that referenced this pull request May 12, 2025
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.

2 participants