-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Comments
We could make the Makefile use
Yep. #411. |
The issue is making a build or cmd flag work with the setting on the UI. We can continue this discussion on #326. |
The env vars thing is what make already does by default :). It’s just that one needs to use Apart from that… yeah 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 |
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 Another tidbit is that you can actually add some meta data to the repo that will be used by 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 |
The build string (with the date) is only used for printing additional context when invoking the $ ./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 |
A rolling $ echo .ARCHIVE_VERSION export-subst > .gitattributes
$ echo '$Format:%h$' > .ARCHIVE_VERSION
$ git add .ARCHIVE_VERSION .gitattributes The |
Yeah, |
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. |
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 ( "directory path of the realpath of the listmonk binary" because I’d install the listmonk binary to |
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 Even better would be a compile time option for the default path to the config file (so we can bake |
Rather for static files there should be a UI option so that the AVG user
does not need to pass a static flag while install to update the template.
…On Wed, 7 Jul, 2021, 8:26 pm Caleb Maclennan, ***@***.***> wrote:
That arrangement would *work* on Arch too. cf. #340
<#340>
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.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#409 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ARESAVF3HFSYCDYFK7ZJ6ETTWRTJDANCNFSM47QCFMAA>
.
|
I understand @knadh sentiment, about the
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 @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 :). |
If @kmohrf makes sense?
@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. |
Hi Sorry for hijacking the thread.
What I meant was only an upload option (similar to images) no editing
option. A customer will make the html on machine and just upload and the
binary can push it to the actual static directory from backend.
…On Thu, 8 Jul, 2021, 11:37 am Kailash Nadh, ***@***.***> wrote:
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 <https://github.com/kmohrf> makes sense?
does not need to pass a static flag while install to update the template.
@jackdanielux <https://github.com/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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#409 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ARESAVFOOA22QIMSIM6NXODTWU6AJANCNFSM47QCFMAA>
.
|
I’m not sure it does, because I think they’re are conflicting usages of the As a packager I would want to provide the As a user I would want to provide the I hope that makes sense. |
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, Given that, I feel it's just simpler to put these 3 files in |
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. |
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 How about making the 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,
|
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:
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 |
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 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 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. 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 |
Yes to all that ^^^. |
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.
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.
The new static-dir branch incorporates the following behaviour. @kmohrf could you please pull and compile it and give a shot?
This is fixed with the new CWD behaviour. Minor gripe: Binaries compiled with these flags with custom app and frontend dirs (eg: |
Not sure if I’m doing something wrong but I’m seeing this error: {"message":"file does not exist"} when loading the frontend on 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 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
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 |
|
That doesn’t seem to change anything. The error message I get is the same. Can I increase logging in listmonk somehow? |
|
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. |
Hence, it's only a minor gripe :)
The only way to pass is via the |
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 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'\"" |
Yep, makes sense 👍 |
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. |
@kmohrf The |
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! |
@kmohrf this should work. Like @alerque suggested, a
The |
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.
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
LAST_COMMIT
andVERSION
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 inMakefile
. 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: callinglistmonk --config /etc/listmonk/mysite.toml --new-config
should create the config in/etc/listmonk/mysite.toml
instead of storing it asconfig.toml
in the current working directory.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
The text was updated successfully, but these errors were encountered: