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

packaging needs #409

Closed
kmohrf opened this issue Jun 29, 2021 · 35 comments
Closed

packaging needs #409

kmohrf opened this issue Jun 29, 2021 · 35 comments
Labels
enhancement New feature or request

Comments

@kmohrf
Copy link
Contributor

kmohrf commented Jun 29, 2021

Is your feature request related to a problem? Please describe.

There are a few issues I’ve bumped into while packaging listmonk for debian(-based) systems. I adressed some of those in this PR. Maybe we can use this issue to explore some of the remaining stumbling blocks.

Describe the solution you'd like

  • The LAST_COMMIT and VERSION make variables are currently hardcoded to use git for determining their respective values. Package builds often use release tarballs which do not contain any git metadata. The easiest solution is to make the caller determine these values by using the ?= instead of the := assignment in Makefile. This is what I do in my deb packaging. The downside is that every packager has to do that manually. A lot of other projects store release metadata in the repository.
  • --new-config should respect the --config option. Pratically speaking: calling listmonk --config /etc/listmonk/mysite.toml --new-config should create the config in /etc/listmonk/mysite.toml instead of storing it as config.toml in the current working directory.
  • I know that Prevent app from update #326 and Please add a flag / config option to not "phone home" #344 already pointed this out, but I want to emphasize it again: I think it would be nice if update checks could be disabled either via the application configuration file or – even better – during build time. It is very common for packagers to disable these checks. There shouldn’t be a need to even render the update-check switch in the frontend in both of these cases.
  • I’d be nice to have a default systemd unit file as part of the project that can serve as a basis for package maintainers. systemd provides a lot of options that reduce the attack surface of an application and it would be nice if we could document any practical and applicable options right here, so they’re discoverable for everyone.

As @alerque seems to be packaging listmonk for ArchLinux he can probably weigh in on this discussion as well. Consider it a friendly request for comments :).

I’d be happy to submit pull requests if we reach consensus on solutions for any of the raised issues. Thank you for your time!

Cheers,

Konrad

@kmohrf kmohrf added the enhancement New feature or request label Jun 29, 2021
@knadh
Copy link
Owner

knadh commented Jun 29, 2021

The LAST_COMMIT and VERSION make variables are currently hardcoded to use git for determining their respective values. Package builds often use release tarballs which do not contain any git metadata. The easiest solution is to make the caller determine these values by using the ?= instead of the := assignment in Makefile. This is what I do in my deb packaging. The downside is that every packager has to do that manually. A lot of other projects store release metadata in the repository.

We could make the Makefile use LAST_COMMIT and VERSION env vars if they are present and fall back to getting the metadata from git otherwise. There can be a VERSION file, but is there a convention for appending build/commit with the semver in it?

--new-config should respect the --config option. Pratically speaking: calling listmonk --config /etc/listmonk/mysite.toml --new-config should create the config in /etc/listmonk/mysite.toml instead of storing it as config.toml in the current working directory.

This is a new feature. Straightforward to add. #410. Done 5e2c24b.

I’d be nice to have a default systemd unit file as part of the project that can serve as a basis for package maintainers. systemd provides a lot of options that reduce the attack surface of an application and it would be nice if we could document any practical and applicable options right here, so they’re discoverable for everyone.

Yep. #411.

@knadh
Copy link
Owner

knadh commented Jun 29, 2021

I know that Prevent app from update #326 and Please add a flag / config option to not "phone home" #344 already pointed this out, but I want to emphasize it again: I think it would be nice if update checks could be disabled either via the application configuration file or – even better – during build time. It is very common for packagers to disable these checks. There shouldn’t be a need to even render the update-check switch in the frontend in both of these cases.

The issue is making a build or cmd flag work with the setting on the UI. We can continue this discussion on #326.

@kmohrf
Copy link
Contributor Author

kmohrf commented Jun 29, 2021

We could make the Makefile use LAST_COMMIT and VERSION env vars if they are present and fall back to getting the metadata from git otherwise. There can be a VERSION file, but is there a convention for appending build/commit with the semver in it?

The env vars thing is what make already does by default :). It’s just that one needs to use ?= instead of := so that they’re not overwritten.

Apart from that… yeah LAST_COMMIT is tricky. semver supports build metadata in the version string, separated by a plus sign. So something like 1.1.0+$SHORTHASH would technically be possible and could be used to determine both the LAST_COMMIT and VERSION variables. The question is what $SHORTHASH would refer to, because the hash is obviously generated after you commit the VERSION file. I actually liked what @alerque did in the Arch PKGBUILD file… so maybe this?

VERSION ?= $(shell git describe --tags --abbrev=0)
BUILDSTR ?= $(VERSION) (\#$(shell git rev-parse --short HEAD) $(shell date -u +"%Y-%m-%dT%H:%M:%S%z"))

This way people know they can override override VERSION and BUILDSTR and that it’s expected behaviour to do so. I was a little worried that BUILDSTR is expected to be a certain format so I was hesitant to override it. I inlined the value of LAST_COMMIT because it’s kind of weird to set a value for it but it would be necessary to stop git from printing warnings when running the build for a tarball.

@alerque
Copy link
Contributor

alerque commented Jun 29, 2021

I'm sorry I'm a bit swamped right now and haven't had time to contribute some followups that come to mind from @kmohrf's earlier PR, but at the very least I can emphatically "second" everything suggested here.

Just a couple quick notes, I suggest avoiding $(shell ...) in Makefiles. I saw this in the PR and didn't have time to comment before it got merged, but thata is an GNU specific function that will trip up builds on BSD system tooling. It's handy as heck, but should only be used when you can guarantee GNU tooling. The existing usage can be fixed uisg $(wildcarad ...), and for the version string I suggest a make target such as .VERSION: with an actual rule for generating the file, then using it's contents instead of a string variable. This will be more portable in the long run. There are some other bits that need fixing for BSD, but at least we don't need to keep adding more GNUisms.

Another tidbit is that you can actually add some meta data to the repo that will be used by git archive to embed commit info in the tarballs it generates (including the GitHub generated ones). This can be used to pre-poppulate more source data into the version string without needing to do full source release builds.

For the build string, I had played with it some and discovered it was free-form, so the Arch Linux packaging just has something I made up. It would be nice to standardize that though release builds across distros identify the distro and packaging versions by default the same way, and also that Git builds identify in a consistent way.

Note also there is a VUE_APP_VERSION that currently has to be exported separately, that should be brought in line with however we handle VERSION.

@knadh
Copy link
Owner

knadh commented Jun 29, 2021

Apart from that… yeah LAST_COMMIT is tricky. semver supports build metadata in the version string, separated by a plus sign. So something like 1.1.0+$SHORTHASH would technically be possible and could be used to determine both the LAST_COMMIT and VERSION variables.

The build string (with the date) is only used for printing additional context when invoking the --version flag. Doesn't make sense to include the hash in the semver.

$ ./listmonk --version                                                                                                                                                                    
v1.0.0 (#34f3ed8 2021-06-29T18:13:59+0000)

If it's going to be a pain when packaging for different enviroments, we can consider ditching it altogether and only printing the semver. Can introduce a rolling develop branch and make master always be at the last annotated tag ensuring that builds always reflect the latest semver, negating the need for the the additional context. However, if there's a bleeding edge / nightly build system based on develop, it'll miss this context.

@alerque
Copy link
Contributor

alerque commented Jun 29, 2021

A rolling develop branch vs. static master (merged on release only) is not necessary at all. In fact for distro packaging purposes the semver data can typically come from the package data anyway. The hash doesn't have to be part of semver either, if it's just stuffed in an extended info stream like that deriving it from Git is fine, supplied via an extra compile time variable or flag for where Git sources are not used. Or add Git commit into to all source archives

$ echo .ARCHIVE_VERSION export-subst > .gitattributes
$ echo '$Format:%h$' > .ARCHIVE_VERSION
$ git add .ARCHIVE_VERSION .gitattributes

The .ARCHIVE_VERSION file found inside the source download links on GitHub will henceforth include that file with short hash of the commet that can be used to inform the version string.

@knadh
Copy link
Owner

knadh commented Jun 30, 2021

Yeah, export-subst works with git archive but it would be nice to have a consistent scheme for git clone also. Otherwise, we've to document two different ways of extracting metadata depending on the mode of download.

@alerque
Copy link
Contributor

alerque commented Jun 30, 2021

You don't have to document it, the tooling can easily detect whether it is a Git clone and use that if present, otherwise fall back to the meta data file stuffed in archives. One or the other of those two things will always be available so the Makefile logic shouldn't be very hard and no manual intervention is required.

@kmohrf
Copy link
Contributor Author

kmohrf commented Jul 7, 2021

While looking at the possibility to externalize the static files in my deb build I noticed that listmonk tries to resolve static filepaths relative to the current working directory (config.toml.example, queries.sql, schema.sql, ...). May I suggest to change that to the directory path of the realpath of the listmonk binary? This way the current working directory can be used for variable files (like uploaded content) and the directory where the listmonk binary is placed is used for static data, that the average user won’t normally modify.

"directory path of the realpath of the listmonk binary" because I’d install the listmonk binary to /usr/share/listmonk/listmonk, symlink it to /usr/bin/listmonk and place any static files into /usr/share/listmonk. Without the realpath static files would be expected in /usr/bin.

@alerque
Copy link
Contributor

alerque commented Jul 7, 2021

That arrangement would work on Arch too. cf. #340, #361

I would personally prefer a completely separate option so that all three paths can be configured at runtime independently. I would prefer to package the binary itself as /usr/bin/listmonk rather than than being a symlink. The args specified at run time should be able to control where it looks for all of these things separately without CWD mattering so much (although it's an okay fallback for them if not specified).

Even better would be a compile time option for the default path to the config file (so we can bake /etc/listmonk.conf or whatever into the distro packaging) and then allow the config file to specify default paths for the other two options. This would keep users from potentially needed to setup systemd service overrides to change paths.

@jackdanielux
Copy link

jackdanielux commented Jul 7, 2021 via email

@kmohrf
Copy link
Contributor Author

kmohrf commented Jul 7, 2021

I understand @knadh sentiment, about the config.toml.sample being part of the app (same goes for queries.sql and schema.sql). Personally I think it’s fine to embed those files in the binary, it’s just that the dist target includes a lot more than integral static files. Specifically the web frontend, which I feel can be packaged separately from the listmonk binary, and files that are obviously supposed to be overridden by users like translations and templates. My suggestion was motivated by the idea to make the smallest non-breaking change to listmonk. But maybe @knadh can just extend the list of lookup paths for files? That would also be backwards compatible. Something like

file:https://$OPTIONAL_PATH_CONFIGURABLE_IN_CONFIG/foo
file:https://$CWD/foo
file:https://$BIN_DIR/foo
file:https:///usr/share/listmonk/foo
embedded:https://foo

Aside from that: In my systemd service unit PR I’ve implemented a template unit. So I’m not really sure about the build-time config variable path, but I guess that’s fine as a default as long as the --config option still works.

@jackdanielux that is a valid concern, but I think you should open up a separate issue for that, as this ticket concentrates on making listmonk packaging easier. At least that was my intention when opening it :).

@knadh
Copy link
Owner

knadh commented Jul 8, 2021

If config.toml.sample, queries.sql, and schema.sql are moved into the static dir, this issue can be simplified without adding any additional params or complexity. To maintain backwards compatibility where --static-dir was only web assets, the existence of these three files in the static-dir can be made optional, so that if they're not present, the bundled versions (if present) are used. This way, existing installations that use --static-dir don't break. This will also eliminate the need for introducing multiple lookup paths.

@kmohrf makes sense?

does not need to pass a static flag while install to update the template.

@jackdanielux a full fledged UI template editor and file manager, which will also require disk permissions, I don't think the effort is worth it. Copying the static-dir and passing that flag is an okay trade-off even for average users, I feel.

@jackdanielux
Copy link

jackdanielux commented Jul 8, 2021 via email

@kmohrf
Copy link
Contributor Author

kmohrf commented Jul 8, 2021

If config.toml.sample, queries.sql, and schema.sql are moved into the static dir, this issue can be simplified without adding any additional params or complexity. This will also eliminate the need for introducing multiple lookup paths.

I’m not sure it does, because I think they’re are conflicting usages of the --static-dir option.

As a packager I would want to provide the --static-dir option to configure the location of a) the sample configuration, schema and queries (actual static data) as well as the default templates and b) the frontend assets (also static, but likely packaged separately).

As a user I would want to provide the --static-dir option to override the default templates. But if the --static-dir option can only be set once, I’d also override things like the location of the queries.sql file. That seems counterintuitive and I’d be back to square one, where I would have to integrate all assets into the binary to make them easy to override.

I hope that makes sense.

@knadh
Copy link
Owner

knadh commented Jul 8, 2021

hm, I think this maybe overthinking it. The SQL files are essentially program code. They're separate files for clarity and ease of development and management. They could've been strings in .go files as well, but at the cost of syntax highlighting, linting etc.

These files will ideally never need to be edited independently of the program code outside of the dev environment. They are completely irrelevant to the end user. Similarly, config.toml.sample's sole purpose is to make --new-config work and is not meant to be user editable. Also, this is only a problem in environments that don't allow embedding arbitrary "static" files in the binary.

Given that, I feel it's just simpler to put these 3 files in static-dir rather than introducing new flags that don't offer any meaningful customization to the end user.

@alerque
Copy link
Contributor

alerque commented Jul 8, 2021

rather than introducing new flags that don't offer any meaningful customization to the end user.

Not all flags are expected to offer end users customization! My suggestion was not for run time flags anyway but configure/compile time options so that the package could be setup the way the distro and users of that distro expect packages to work.

Having sql code files that are truly part of the app internals mixed in with html templates that the user can point to their own location and supply there is is not a good idea.

@knadh I think you're forgetting there that we're not embedding anything. Your logic seems to depend on having embedded assets to fall back on, hence sparse static directories being user facing is okay. If the embedding was only for loading up static string assets that would be one thing, but it isn't. Since there is stuff in there we can't really pass off in a distro package, we're kind of forced into embedding nothing, in which case the static directly starts having multiple personality issues. There should really be two places. I don't care if upstream defaults to embedding and mixing everything together. But it would be really nice to be able to package this in two places: one for the assets that are really static assets and shouldn't be messed with, and one with assorted html and templates that the user can reconfigure in the config file to point to their own assets if they want.

@knadh
Copy link
Owner

knadh commented Jul 8, 2021

Not all flags are expected to offer end users customization! My suggestion was not for run time flags anyway but configure/compile time options so that the package could be setup the way the distro and users of that distro expect packages to work.

Of course I understand this. Just trying to come up with something that does not involve addition of new runtime flags (which right now are entirely for user config), or compile time flags (that have edge cases like @kmohrf mentioned), or config.toml, which again is for user configuration (avoiding a path-to-sql-files-that-are-critical key).

How about making the --static-dir flag a slice like --config where each given dir merges into the previous ones?

For envrionments that need to load files separately, it would look like:

listmonk --static-dir=/path/to/sql-files --static-dir=/path/to/ui-files

# or even

listmonk --static-dir=/path/to/sql-files --static-dir=/path/to/sample-config --static-dir=/path/to/ui-files

For users who run the binary with the embedded files, the current behaviour doesn't change.

listmonk --static-dir=/path/to/custom-ui-files

So, --static-dir is a way to supply N directory paths that in the end should make up a directory with the all the files and dirs the program expects:

/static
	schema.sql
	queries.sql
	config.toml.sample
	public/
	email-templates/

@alerque
Copy link
Contributor

alerque commented Jul 8, 2021

I find making the dir a "slice" (is that a Golang term?) that overlay file sets with each evocation weird and slightly counter intuitive, but it would get the job done in this case.

The one caveat is I'm not sure how nicely this would play with the config file. Would this allow the following use case as a distro package:

  1. Bundle no static assets.
  2. Package the SQL and other truly static assets in one place.
  3. Package the assorted templates in a second place
  4. Add two --static-dir options to the packaged systemd runtime file, one for each
  5. Allow the user to add a static-dir to their config.toml to pass their own customized templates?

This would allow package integrity things to pass nicely. Remember letting users modify packaged files is a big no-no. Without №5 above users would have to ditch using any of the packaged templates to customize just one of them. That's

@kmohrf
Copy link
Contributor Author

kmohrf commented Jul 8, 2021

God I swear I have rewritten this comment at least six times. 😅

I think there are three distinct problems here.

Firstly the SQL files and the sample config. I don’t think it’s a bad thing if these are embedded. Like @knadh stated, they are an integral part of the application and even though it might be interesting to look at them I don’t see a whole lot of reason for anyone to do so. I do think it’s weird, that they’re not embedded during the build make target, but that’s another story. I don’t think that --static-dir is a good solution here simply because a misplaced cli option should not break the internal state of the application. In case they’re not embedded a build time variable (i.e. APPLICATION_FILE_ROOT=/usr/share/listmonk) is the way to ~Go~ (badumtss) here IMHO. If the build time variable is not set fallback to CWD.

Secondly there are the templates and the i18n files. I don’t think these should be embedded, at least not when packaging for an operating system. This way it’s easy for people to simply copy the files from the location where the package installed them to a place where they can be edited (for instance /etc/listmonk/foo/ or /var/lib/private/listmonk-foo/). Making multiple --static-dir options a thing would be nice in order to support partial template overrides, so that --static-dir foo --static bar would make listmonk fallback to foo/index.html if bar/index.html does not exist. But that’s just nice to have.

Thirdly there are the frontend assets. I was sure that these were part of the static directory, but they’re not. A build time variable (i.e. FRONTEND_FILE_ROOT=/usr/share/listmonk-webapp ) would be fine for those too, just not the same like the one above, because the assets would often would be part of a separate and optional package. If the build time variable is not set fallback to CWD.

I guess that is what @alerque was thinking about in the beginning, so uhm… sorry for the confusion.

Sidenotes: I see that @alerque externalized the frontend assets, but this only seems to work if no embedding happens at all. I’ve tried to embed only the SQL files and the sample config and then start the app with externalized frontend assets in the current working directory but then listmonk just reports {"message":"file does not exist"} and I must pass the --i18n and --static-dir options to make it even run in the first place and despite the fact that the respective directories are located in the current working directory.

@alerque
Copy link
Contributor

alerque commented Jul 8, 2021

Yes to all that ^^^.

knadh added a commit that referenced this issue Jul 11, 2021
Ref: #409

- Introduce `main.appDir` and `main.fronendDir` Go compile-time flags
  to hardcode custom paths for loading frontend assets
  (frontend/dist/frontend in the repo after build) and app assets
  (queries.sql, schema.sql, config.toml.sample) in environments where
  embedding files in the binary is not feasible.
  These default to CWD unless explicitly set during compilation.

- Fix the Vue favicon path oddity by copying the icon into the built
  frontend dir in the `make-frontend` step.
knadh added a commit that referenced this issue Jul 11, 2021
Ref: #409

- Introduce `main.appDir` and `main.fronendDir` Go compile-time flags
  to hardcode custom paths for loading frontend assets
  (frontend/dist/frontend in the repo after build) and app assets
  (queries.sql, schema.sql, config.toml.sample) in environments where
  embedding files in the binary is not feasible.
  These default to CWD unless explicitly set during compilation.

- Fix the Vue favicon path oddity by copying the icon into the built
  frontend dir in the `make-frontend` step.
@knadh
Copy link
Owner

knadh commented Jul 11, 2021

The new static-dir branch incorporates the following behaviour. @kmohrf could you please pull and compile it and give a shot?

  • Introduces two new compile-time flags (passed to go in -ldflags):
    -X 'main.appDir=/path' and -X 'main.frontendDir=/path'

  • These default to the CWD and only kick in when no embedded files are found. appDiris expected to contain config.toml.sample, schema.sql, queries.sql and frontendDir is expected to contain frontend/dist/frontend/* (the result of make build-frontend).

  • No change in the behaviour of binaries with embedded files.

  • No change in the behaviour of --static-dir and --i18n-dir runtime flags.

I must pass the --i18n and --static-dir options to make it even run in the first place and despite the fact that the respective directories are located in the current working directory.

This is fixed with the new CWD behaviour.

Minor gripe: Binaries compiled with these flags with custom app and frontend dirs (eg: /usr/share/listmonk), on their own, become less "portable" when copied to different environments, forcing users to create these directories that require elevated permissions.

@kmohrf
Copy link
Contributor Author

kmohrf commented Jul 11, 2021

Not sure if I’m doing something wrong but I’m seeing this error:

{"message":"file does not exist"}

when loading the frontend on https://localhost:9000/.

I build listmonk (I did not embed any files with stuffbin) with:

CGO_ENABLED=0 go build -o listmonk -ldflags="-s -w -X 'main.buildString=test (#82735bb 2021-07-11T10:00:23+0000)' -X 'main.versionString=test' -X 'main.appDir=/usr/share/listmonk' -X 'main.frontendDir=/usr/share/listmonk-webapp'" cmd/*.go

I started listmonk from the /var/lib/listmonk directory, with the following arguments:

listmonk --config /etc/listmonk/newsletter.toml --i18n-dir /usr/share/listmonk/i18n --static-dir /usr/share/listmonk/static

The very minimal chroot I run listmonk from looks like this (I stripped /proc for brevity):

.
├── etc
│   ├── listmonk
│   │   └── newsletter.toml
│   └── resolv.conf
├── usr
│   ├── bin
│   │   ├── busybox
│   │   └── listmonk
│   └── share
│       ├── listmonk
│       │   ├── i18n
│       │   │   ├── de.json
│       │   │   ├── en.json
│       │   │   ├── es.json
│       │   │   ├── fr.json
│       │   │   ├── it.json
│       │   │   ├── ml.json
│       │   │   ├── pl.json
│       │   │   ├── pt-BR.json
│       │   │   ├── pt.json
│       │   │   ├── ru.json
│       │   │   └── tr.json
│       │   ├── static
│       │   │   ├── email-templates
│       │   │   │   ├── base.html
│       │   │   │   ├── campaign-status.html
│       │   │   │   ├── default.tpl
│       │   │   │   ├── import-status.html
│       │   │   │   ├── subscriber-data.html
│       │   │   │   ├── subscriber-optin-campaign.html
│       │   │   │   └── subscriber-optin.html
│       │   │   └── public
│       │   │       ├── static
│       │   │       │   ├── favicon.png
│       │   │       │   ├── logo.png
│       │   │       │   ├── logo.svg
│       │   │       │   ├── script.js
│       │   │       │   └── style.css
│       │   │       └── templates
│       │   │           ├── index.html
│       │   │           ├── message.html
│       │   │           ├── optin.html
│       │   │           ├── subscription-form.html
│       │   │           └── subscription.html
│       │   ├── config.toml.sample
│       │   ├── queries.sql
│       │   └── schema.sql
│       └── listmonk-webapp
│           └── frontend
│               └── dist
│                   ├── frontend
│                   │   ├── css
│                   │   │   ├── app.7925574c.css
│                   │   │   └── main.fd985d68.css
│                   │   ├── fonts
│                   │   │   └── fontello.d354fd47.woff2
│                   │   ├── img
│                   │   │   └── logo.b296cf65.svg
│                   │   ├── js
│                   │   │   ├── app.0a64dce9.js
│                   │   │   ├── chunk-vendors.07e6dcf2.js
│                   │   │   └── main.92baf027.js
│                   │   └── index.html
│                   └── favicon.png
└── var
    └── lib
        └── listmonk

I did all this on ArchLinux with go 1.16.5 (I only use debian on servers and for the final builds). When I use strace -f to analyze file operations performed by listmonk I see that it traverses the files in the listmonk-webapp directory so it does seem to find them. I’m not really sure which file the file does not exist error actually refers to. Do you have any thoughts?

@knadh
Copy link
Owner

knadh commented Jul 11, 2021

main.frontendDir should be the root directory with of the frontend assets. In your example, it should be main.frontendDir=/usr/share/listmonk-webapp/frontend

@kmohrf
Copy link
Contributor Author

kmohrf commented Jul 11, 2021

That doesn’t seem to change anything. The error message I get is the same. Can I increase logging in listmonk somehow?

@kmohrf
Copy link
Contributor Author

kmohrf commented Jul 11, 2021

/usr/share/listmonk-webapp/frontend/dist/frontend did the trick, which is nice, because my next question would have been, if it would be possible to put the content of frontend/dist/frontend directly into the frontendDir directory, which seems to be the case already. Perfect!

@kmohrf
Copy link
Contributor Author

kmohrf commented Jul 11, 2021

Minor gripe: Binaries compiled with these flags with custom app and frontend dirs (eg: /usr/share/listmonk), on their own, become less "portable" when copied to different environments, forcing users to create these directories that require elevated permissions.

I guess that package maintainers are the only ones who would use these build time flags and users of those packages will most likely not have any expectation of portability nor the incentive to move these files around. Anyone else will have a perfect time with your releases from GitHub :).

What’s a good way to pass the ldflags to the build process? Does the go compiler support an environment variable that allows us to set the required flags? If not I’d be nice to have a make variable to pass additional go compiler arguments or ldflags.

@knadh
Copy link
Owner

knadh commented Jul 11, 2021

I guess that package maintainers are the only ones who would use these build time flags and users of those packages will most likely not have any expectation of portability nor the incentive to move these files around. Anyone else will have a perfect time with your releases from GitHub :).

Hence, it's only a minor gripe :)

What’s a good way to pass the ldflags to the build process? Does the go compiler support an environment variable that allows us to set the required flags? If not I’d be nice to have a make variable to pass additional go compiler arguments or ldflags.

The only way to pass is via the -ldflags CLI flag to go build. To use environment variables, we still have to construct the CLI flags and then inject variables in there, eg: go build -ldflags="-X 'main.someVar=${ENV_VAR}'".

@kmohrf
Copy link
Contributor Author

kmohrf commented Jul 11, 2021

Maybe we can do a something like (shortened for brevity):

GO_BUILD_ARGS ?=

$(BIN): $(shell find . -type f -name "*.go")
    CGO_ENABLED=0 go build -o ${BIN} -ldflags="-s -w -X 'main.buildString=${BUILDSTR}' -X 'main.versionString=${VERSION}'" $(GO_BUILD_ARGS) cmd/*.go

Go doesn’t seem to mind multiple -ldflags options and this allows for any kind of addtional compiler args.

This way I can just call:

make build GO_BUILD_ARGS="-ldflags=\"-X 'main.appDir=/usr/share/listmonk' -X 'main.frontendDir=/usr/share/listmonk-webapp'\""

@knadh
Copy link
Owner

knadh commented Jul 11, 2021

Yep, makes sense 👍

@alerque
Copy link
Contributor

alerque commented Jul 12, 2021

Minor gripe: Binaries compiled with these flags with custom app and frontend dirs (eg: /usr/share/listmonk), on their own, become less "portable" when copied to different environments, forcing users to create these directories that require elevated permissions.

This is expected. People do not expect bits of packages installed via system package managers to be portable. Portable apps are a whole genre of their own and it's totally okay for system packages to not be portable. It's also fine that you have a way to build portable stand alone versions, but this isn't a downside for distros.

@knadh
Copy link
Owner

knadh commented Jul 25, 2021

@kmohrf The static-paths branch with the new build flags is now merged into master.

@kmohrf
Copy link
Contributor Author

kmohrf commented Aug 4, 2021

Nice to see things falling into place :). I think the last remaining issue is the build string in the application version for which @alerque already pointed out a solution in his comment that includes the string in the generated git archive. If this works out I suggest we close this ticket :). yay!

It was a pleasure working with you @knadh. Thank you for your efforts!

@knadh
Copy link
Owner

knadh commented Aug 8, 2021

@kmohrf this should work. Like @alerque suggested, a VERSION file is generated during git archive that will have the following two lines, the commit hash and the tag.

a217267
HEAD -> version-file, tag: v1.1.1

The LAST_COMMIT and VERSION variables in the Makefile will try to get the hash using git or fall back to extracting from the VERSION file. Once you get a chance to check this and confirm, will merge.

@knadh
Copy link
Owner

knadh commented Aug 14, 2021

@kmohrf this is now merged to master: d27e16e. Tested make build and make dist in git, non-git (git archive), and goreleaser environments and it works as expected. Hope this simplifies packaging. Closing this issue now. Thanks for your time!

@knadh knadh closed this as completed Aug 14, 2021
Pannigliaet added a commit to Pannigliaet/listmonk that referenced this issue Aug 27, 2024
Ref: knadh/listmonk#409

- Introduce `main.appDir` and `main.fronendDir` Go compile-time flags
  to hardcode custom paths for loading frontend assets
  (frontend/dist/frontend in the repo after build) and app assets
  (queries.sql, schema.sql, config.toml.sample) in environments where
  embedding files in the binary is not feasible.
  These default to CWD unless explicitly set during compilation.

- Fix the Vue favicon path oddity by copying the icon into the built
  frontend dir in the `make-frontend` step.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants