search

KJ7LNW / xnec2c

Xnec2c is a high-performance multi-threaded electromagnetic simulation package to model antenna near- and far-field radiation patterns for Linux and UNIX operating systems.
https://www.xnec2c.org/
GNU General Public License v3.0
78 stars 17 forks source link

Buildsystem cleanups #17

Closed ndim closed 1 year ago

ndim commented 1 year ago

This tries to fix a few obvious problems with the build system, such as

I am creating this as a draft pull request as I have a few questions about the dependency tracking for built sources which I still need to clarify.

(FYI, I am not an xnec2c user. I am just running into questions about its build system on stackoverflow again and again.)

KJ7LNW commented 1 year ago

Hi Hans, thanks for the contribution! It is nice to get the build system cleaned up.

I will look at this in detail over the next few days and let you know if I have any other questions or comments. By the way, what motivated your interest in the build system cleanup?

Thanks again!

-Eric

KJ7LNW commented 1 year ago

See above, too, but clearly you are more familiar with autoconf and its interaction with glib-compile-resources.

Question: Is it best practice to distribute the files in resources/ for users at make install time, even though they are built directly into the xnec2c binary? If so, why would users want build-time files (like, for example, xnec2c.gresource.xml)?

ndim commented 1 year ago

Why require automake >= 1.13?

Because you have already been using AC_CONFIG_MACRO_DIRS() for a long time (note the plural S at the end) for which an important bug was fixed in automake 1.13.

Also note that automake-1.13 was released in 2012-12-28, which is quite close to the 2012-04-25 release of autoconf-2.69 from a 2022 perspective - and you have had AC_PREREQ([2.69]) in configure.ac since commit 56d022c0 from 2013, so you have already been requiring tools from the 2012 era for a good nine years. So this does significantly change the vintage of build tools xnec2c requires.

Side note: I have found either libtool or gettext to not properly handle AC_CONFIG_MACRO_DIRS (with the letter S) and have reverted to using a combination AC_CONFIG_MACRO_DIR and ACLOCAL_AMFLAGS in other projects. I will need to look that up, at least if you want actually use that po/ subdir and the translations for xnec2c.

Why distribute the files inside the resources/ directory?

I think you are confusing two things:

Why am I interested in this build system cleanup?

Observing you asking buildsystem questions on stackoverflow again and again over the past months and years interested me into helping more directly.

KJ7LNW commented 1 year ago

Awesome, I appreciate it! Thanks for clarifying my various questions.

About AC_CONFIG_MACRO_DIRS(): If AC_CONFIG_MACRO_DIR() without the "s" would be better then I'm all for it!

KJ7LNW commented 1 year ago

All of my systems are based on CentOS 7 and probably will be for at least the next year or so, which is the reason for the Autoconf 2.69 target. I tried running autogen.sh (which should probably be cleaned up, too, but that isn't the issue here) and we get the following errors:

autogen.sh

]# ./autogen.sh 
Preparing the xnec2c build system...please wait

Found GNU Autoconf version 2.69
Found GNU Automake version 1.13.4
Found GNU Libtool version 2.4.2

Automatically preparing build ... Warning: autoreconf failed
Attempting to run the preparation steps individually

Preparing build ... 
Warning:  Unsupported macros were found in ./configure.ac

The configure.ac file was scanned in order to determine if any
unsupported macros are used that exceed the minimum version
settings specified within this file.  As such, the following macros
should be removed from configure.ac or the version numbers in this
file should be increased:

AC_FUNC_REALLOC

Ignorantly continuing build preparation ... 
Warning: autoconf seems to have succeeded by removing the following options:
    AUTOCONF_OPTIONS="-f"

Removing those options should not be necessary and indicate some other
problem with the build system.  The build preparation is highly suspect
and may result in configuration or compilation errors.  Consider
rerunning the build preparation with verbose output enabled.
    ./autogen.sh --verbose

Continuing build preparation ... done

The xnec2c build system is now prepared.  To build here, run:
  ./configure
  make

./configure

Then running ./configure I get this:

]# ./configure
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /usr/bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking whether make supports nested variables... yes
checking whether to enable maintainer-specific portions of Makefiles... no
checking build system type... x86_64-unknown-linux-gnu
checking host system type... x86_64-unknown-linux-gnu
checking whether configure should try to set CFLAGS... yes
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables... 
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking for style of include used by make... GNU
checking dependency style of gcc... gcc3
checking whether make sets $(MAKE)... (cached) yes
checking for glib-compile-resources... ./configure: line 4031: syntax error near unexpected token `GLIB_COMPILE_RESOURCES,'
./configure: line 4031: `PKG_CHECK_VAR(GLIB_COMPILE_RESOURCES, gio-2.0, glib_compile_resources)'

According to Red Hat, they do not ship PKG_CHECK_VAR with pkgconfig-0.27.1-4 which explains why grep PKG_CHECK_VAR /usr/share/aclocal/pkg.m4 returns nothing.

Do you have a work-around for PKG_CHECK_VAR?

ndim commented 1 year ago

Quick note: This is not ready for merge, for a number of reasons. That is why I filed this as a Draft PR.

I am still clarifying some details regarding the auto dependencies for the resource compilation.

Regarding AC_CONFIG_MACRO_DIRS: Automake appear to have deprecated AC_CONFIG_MACRO_DIR (without S) in favour of AC_CONFIG_MACRO_DIR (with S), but last time I checked, that news had not reached the real world gettext/libtool (whichever of the two it was). I want to check the details of as well before this can be merged.

Regarding autogen.sh: What does autogen.sh do which autoreconf does not do? If the answer to that is "nothing", I would remove autogen.sh, or replace it with exec autoreconf --install --force. However, I have not checked what your autogen.sh does - it is a huge amount of code to read and for my purposes, autoreconf just did the job.

And let me try building on CentOS 7 (my Fedora 37 system has a mock config for CentOS 7) and a few more systems.

ndim commented 1 year ago

As to PKG_CHECK_VAR, that can be worked around easily if necessary.

KJ7LNW commented 1 year ago

I'm guessing that autoreconf --install --force is fine...or at least, it builds ok with these warnings:

]$ autoreconf --install --force
src/Makefile.am:63: warning: shell which pkg-config: non-POSIX variable name
src/Makefile.am:63: (probably a GNU make extension)
src/Makefile.am:64: warning: shell $(PKGCONFIG: non-POSIX variable name
src/Makefile.am:64: (probably a GNU make extension)
src/Makefile.am:66: warning: shell $(GLIB_COMPILE_RESOURCES: non-POSIX variable name
src/Makefile.am:66: (probably a GNU make extension)

I inherited the code as the new maintainer from Neoklis (the original developer) and I think he was using a default autoconf stack way back in 2008. Looking at the header it appears that autogen.sh was canned and copied by Neoklis for this project since the original authors have nothing to do with xnec2c:

# Author:
#   Christopher Sean Morrison <morrison@brlcad.org>
#
# Patches:
#   Sebastian Pipping <sebastian@pipping.org>

The only exception to to this is that I've added some checks for missing packages that autogen.sh needed to succeed to help xnec2c users build the package without opening a github issue. These are the only changes I've made ti autogen.sh for that purpose and perhaps there is a better way to do these sanity checks in configure.ac instead shell script hacks in autogen.sh:

if [ ! -x "$(command -v gettext)" ]; then
        echo "gettext does not exist: install the gettext package."
        exit 1
fi

if [ ! -x "$(command -v gettextize)" ]; then
        echo "gettextize does not exist: install the gettext package."
        exit 1
fi

if [ ! -x "$(command -v autopoint)" ]; then
        echo "autopoint does not exist: install the autopoint or gettext-devel package."
        exit 1
fi

if [ ! -x "$(command -v glib-compile-resources)" ]; then
        echo "glib-compile-resources does not exist: install the libglib2.0-dev-bin or glib2-devel package."
        exit 1
fi

if [ ! -x "$(command -v pkg-config)" ]; then
        echo "pkgconfig does not exist: install the pkgconfig or pkg-config package."
        exit 1
fi

... so if we can fix whatever is triggering the autoreconf warnings above (and I think you already addressed some of that in your patch) then the simple command checks above (or refactored into configure.ac) with exec autoreconf --install --force would be much better solution!

What do you think?

KJ7LNW commented 1 year ago

Regarding mock builds for el7: there is an xnec2c.spec in the git tree if that is helpful.

Beware of version tagging issues:

There is a v before the version in the package name (like v4.4.12), which probably deviates from the .tar.gz that make dist creates. I plan to drop v from tags starting in version 4.5 because of the issues it creates: this was an unintended consequence of using the Linux Kernel's tagging convention that didn't play well with the .spec's %setup macro and github's automatic /archive/<name>-<tag>.tar.gz generator. I'm open to suggestions here if you care to comment. Feel free to modify the .spec if you see a better way.

Someday I would like to make xnec2c.spec.in as suggested on SE so the @PACKAGE_VERSION@ can be populated easier. You might recognize the author of that answer ;)

I've not gotten around to cleaning up the auto-version stuff yet; right now these are the files that have the version hard-coded that would benefit from a .in version of the file:

ndim commented 1 year ago

Quick note on build tool versions: xnec2c requires gettext version 0.19.7 from 2015-12, which is 3 years younger than automake 1.13 and 3.5 years younger than autoconf 2.69. And that is even without any translations.

ndim commented 1 year ago

Beware of version tagging issues:

There is a v before the version in the package name (like v4.4.12), which probably deviates from the .tar.gz that make dist creates. I plan to drop v from tags starting in version 4.5 because of the issues it creates: this was an unintended consequence of using the Linux Kernel's tagging convention that didn't play well with the .spec's %setup macro and github's automatic /archive/<name>-<tag>.tar.gz generator. I'm open to suggestions here if you care to comment. Feel free to modify the .spec if you see a better way.

Do not rush into changing the git release tags.

github releases (which happen via git tags) versus GNU buildsystem dist tarball releases.

First, https://github.com/KJ7LNW/xnec2c/archive/refs/tags/v4.4.12.tar.gz actually downloads a tarball named xnec2c-4.4.12.tar.gz. However, while this has the same name as the dist tarball the GNU buildsystem creates by make dist or make distcheck, the content is very different:

Unfortunately, you cannot disable the repo dump tarballs on Github releases, but you can attach other binaries to a release, either manually or with a Github workflow. See e.g. the https://github.com/gphoto/libgphoto2/releases/tag/v2.5.30 release: While there is a "Source Code" tarball with the URL v2.5.30.tar.gz which is just the git source tree dump, there are also dist tarballs available in three compression flavours: libgphoto2-2.5.30.tar.{gz,bz2,xz}.

These days, dist tarballs are starting to become less significant as a method for distributing software releases, with Github's automatic source tree dump tarballs or even "just checkout our release branch" beginning to replace dist tarballs.

Both methods have their advantages and disadvantages.

Anyway, I would keep using the git tags for the git releases at v1.2.3 with the v.

Someday I would like to make xnec2c.spec.in as suggested on SE so the @PACKAGE_VERSION@ can be populated easier. You might recognize the author of that answer ;)

That is easy.

I've not gotten around to cleaning up the auto-version stuff yet; right now these are the files that have the version hard-coded that would benefit from a .in version of the file:

* doc/xnec2c.1
* xnec2c.spec

Agreed about those two. They can take their version information from configure.ac via configure and Makefile and some $(SED) substitutions.

BTW, are you married to the recursive make src/Makefile.am or would a single parallel make run (except for po/) also be OK? Will be a bit faster, but you cannot cd src && make any more.

* configure.ac

I do not see what you would want to accomplish with a .in version of this file. Where would the version information to substitute come from?

What is possible is to have autoreconf call autoconf call an m4_esyscmd() macro call a shell script. That shell script can then figure out the version information to put in AC_INIT, e.g. from

However, I do not know whether you want to go that route.

I personally am using such a script in my more or less personal projects like https://github.com/ndim/ndim-git-utils https://github.com/ndim/ndim-utils: They both use two number release tags like e.g. v1.2, and then the script will create a version number for AC_INIT like v1.2.23 for the 23rd commit after the v1.2 tag. I do now know how something like that would fare with many developers and users, though. We had something like that about 15 years ago with the xorg-x11-drv-radeonhd driver, but that was short lived as well.

ndim commented 1 year ago

I'm guessing that autoreconf --install --force is fine...or at least, it builds ok with these warnings:

]$ autoreconf --install --force
src/Makefile.am:63: warning: shell which pkg-config: non-POSIX variable name
src/Makefile.am:63: (probably a GNU make extension)
src/Makefile.am:64: warning: shell $(PKGCONFIG: non-POSIX variable name
src/Makefile.am:64: (probably a GNU make extension)
src/Makefile.am:66: warning: shell $(GLIB_COMPILE_RESOURCES: non-POSIX variable name
src/Makefile.am:66: (probably a GNU make extension)

That is indeed fixed by the "Remove $(shell) parts from src/Makefile.am" commit.

I inherited the code as the new maintainer from Neoklis (the original developer) and I think he was using a default autoconf stack way back in 2008.

[...]

The only exception to to this is that I've added some checks for missing packages that autogen.sh needed to succeed to help xnec2c users build the package without opening a github issue. These are the only changes I've made to autogen.sh for that purpose and perhaps there is a better way to do these sanity checks in configure.ac instead shell script hacks in autogen.sh:

if [ ! -x "$(command -v gettext)" ]; then
        echo "gettext does not exist: install the gettext package."
        exit 1
fi

if [ ! -x "$(command -v gettextize)" ]; then
        echo "gettextize does not exist: install the gettext package."
        exit 1
fi

if [ ! -x "$(command -v autopoint)" ]; then
        echo "autopoint does not exist: install the autopoint or gettext-devel package."
        exit 1
fi

if [ ! -x "$(command -v glib-compile-resources)" ]; then
        echo "glib-compile-resources does not exist: install the libglib2.0-dev-bin or glib2-devel package."
        exit 1
fi

if [ ! -x "$(command -v pkg-config)" ]; then
        echo "pkgconfig does not exist: install the pkgconfig or pkg-config package."
        exit 1
fi

There is no need to check for glib-compile-resources in autogen.sh: configure will check for glib-compile-resources. It might even be useless, as the system you ran autogen.sh on (the one you run make dist on) might not be the one you build on (the one you run configure && make && make install on

You also do not need the pkg-config executable at autogen.sh time. What you do need is pkg.m4, which is usually part of some -devel package.

A standard build will need autopoint from some gettext devel package at autogen.sh time, and gettext at configure and make time, however only developers will ever need gettextize.

I guess I can take a look through past xnec2c github issues and see what can be done with an autoreconf based buildsystem initialization.

... so if we can fix whatever is triggering the autoreconf warnings above (and I think you already addressed some of that in your patch) then the simple command checks above (or refactored into configure.ac) with exec autoreconf --install --force would be much better solution!

What do you think?

autoreconf will complain if e.g. it finds gettext related macros in configure.ac but autopoint cannot be found.

autoreconf will complain if pkg.m4 is not found because m4_pattern_forbid([PKG_...]) will trigger on unexpanded PKG_ macros.

configure will complain if it cannot find glib-compile-resources.

The exact error messages could maybe be better for some situations, but the exact name of the missing packages and how to install them is highly system specific, so you cannot be expected to prepare error messages with detailed instructions suitable to all operating systems.

I would just use autoreconf and configure and their error messages, and depending on the situation, either document it in README.md or INSTALL.md or something similar, or maybe (if reasonable, possible, and useful, and only if it actually causes people issues) put a more detailed error message in place, e.g. using m4_ifndef([MACRO], [m4_fatal([MACRO required ... macro.m4 ... part of foobar dev package])])dnl instead of just m4_pattern_forbid([MACRO]).

ndim commented 1 year ago

Remark on Centos 7 having an old pkg.m4 without PKG_CHECK_VAR: If you create a dist tarball on a contemporary system, the aclocal.m4 inside the dist tarball will contain the PKG_CHECK_VAR from the contemporary system, and configure && make && make install on Centos 7 from a tarball will work.

However, if you have users who build from git on Centos 7, the alternative implementation of PKG_CHECK_VAR inside configure.ac is required, or shipping pkg.m4 inside m4-*/.

KJ7LNW commented 1 year ago

Do not rush into changing the git release tags. github releases (which happen via git tags) versus GNU buildsystem dist tarball releases.

Great information. I'll stick with v on the tag for now.

What is possible is to have autoreconf call autoconf call an m4_esyscmd() macro call a shell script. That shell script can then figure out the version information to put in AC_INIT ... However, I do not know whether you want to go that route.

For now manually editing the configure.ac with the latest version is the way to go in case I miss a tag or have some other tagging problem.

You also do not need the pkg-config executable at autogen.sh time. What you do need is pkg.m4, which is usually part of some -devel package.

Maybe shipping our own pkg.m4 (or even pkg-check-var.m4 with just the missing bits) because my desk will be el7 for a while. I plan to update my desk to Oracle Linux 8 or Oracle Linux 9 but it might still be a while and all my builds are still on el7 for the near feature.

Also I agree with dropping the binary checks in autogen.sh based on your suggestions so feel free to patch it into just an exec autoreconf ... call.

BTW, are you married to the recursive make src/Makefile.am or would a single parallel make run (except for po/) also be OK? Will be a bit faster, but you cannot cd src && make any more.

Either is fine, very rarely do I do something like cd src && make foo.o or whatever. Usually I just make at the top of the tree.

ndim commented 1 year ago

Do not rush into changing the git release tags. github releases (which happen via git tags) versus GNU buildsystem dist tarball releases.

Great information. I'll stick with v on the tag for now.

Consistency (i.e. continuing the existing theme) will also make any future automation and communication MUCH simpler.

What is possible is to have autoreconf call autoconf call an m4_esyscmd() macro call a shell script. That shell script can then figure out the version information to put in AC_INIT ... However, I do not know whether you want to go that route.

For now manually editing the configure.ac with the latest version is the way to go in case I miss a tag or have some other tagging problem.

An m4_esyscmd version script does not need to behave the same as mine does. It is turing complete, so it can do anything. Just keeping using the version from the latest tag and ignoring the commits since could also be a reasonable choice.

But that can always be changed later, we do not need to deal with that now.

You also do not need the pkg-config executable at autogen.sh time. What you do need is pkg.m4, which is usually part of some -devel package.

Maybe shipping our own pkg.m4 (or even pkg-check-var.m4 with just the missing bits) because my desk will be el7 for a while. I plan to update my desk to Oracle Linux 8 or Oracle Linux 9 but it might still be a while and all my builds are still on el7 for the near feature.

Shipping pkg.m4 is a brilliant idea. Why have I not thought about that?

With aclocal's serial mechanism, xnec2c can just contain copy of a modern pkg-config's pkg.m4 in m4-include, and aclocal will always pick the newest pkg.m4 file, be it the one from m4-include/ or the system one from /usr/share/aclocal.

It is also important information that you as the main dev run EL7. I have checked EL7 tool version availability within a mock centos-7 chroot and remarked on what tool versions mean for xnec2c in a comment in configure.ac so that whenever someone needs to deal with tool versions, that information is right there.

Also I agree with dropping the binary checks in autogen.sh based on your suggestions so feel free to patch it into just an exec autoreconf ... call.

I still want to go through past github build issues for ideas there.

BTW, are you married to the recursive make src/Makefile.am or would a single parallel make run (except for po/) also be OK? Will be a bit faster, but you cannot cd src && make any more.

Either is fine, very rarely do I do something like cd src && make foo.o or whatever. Usually I just make at the top of the tree.

Given that you are still on EL7, and that EL7 only has automake 1.13.4 which does not know %reldir% yet (introduced by automake 1.14), I would keep the current recursive three Makefile setup (xnec2c/Makefile xnec2c/src/Makefile xnec2/po/Makefile). The substitutions for xnec2c/doc/xnec2c.1 and xnec2c/xnec2c.spec can be easily dealt with from the top level directory Makefile (or possibly from doc/include.am) with hard coded doc/ in the file names. The same goes for EXTRA_DIST += m4-include/pkg.m4.

KJ7LNW commented 1 year ago

Awesome. This branch builds great on my system.

About past bugs with build issues, see bugs #3 and #6. I thought there were more, maybe from direct email but I couldn't find any.

KJ7LNW commented 1 year ago

About past bugs with build issues, see bugs #3 and #6. I thought there were more, maybe from direct email but I couldn't find any.

Oh, and also see open issues #16 and #14.

KJ7LNW commented 1 year ago

Are you still planning to replace autogen.sh?

I tried exec autoreconf --install --force and get this warning:

xnec2c-git]$ autoreconf --install --force
Copying file ABOUT-NLS
[...]
src/Makefile.am:9: warning: compiling 'main.c' with per-target flags requires 'AM_PROG_CC_C_O' in 'configure.ac'

Aside from the warning, your branch builds and runs fine.

Has anything changed that would effect make desktop-install ?

ndim commented 1 year ago

Are you still planning to replace autogen.sh?

If it was my personal package, I would remove it (IMHO, autogen.sh removal has been overdue for the last 10 to 20 years), but your package, your call.

autogen.sh is one name to remember and to run.

autoreconf -is is one name with possible multiple options to remember and to run.

Keeping autogen.sh means that we keep one part of the user interface ("call autogen.sh"), but we change what that actually does. Not sure how compatible you want to be, or what you want autogen.sh to do what autoreconf does not succeed.

A good compromise might be to

#!/bin/sh

top_srcdir="$(dirname "$0")"

if autoreconf --default --options "$top_srcdir"; then
    true  # all is well that ends well
else
    s=$?
    cat<<EOF

Helpful message explaining the most frequent problems autoreconf can have
and what to do about them, and then pointing to the INSTALL instructions.

E.g. "when autoreconf cannot `find` autopoint, try installing the development
parts of the gettext package"
EOF
    exit "$s"
fi

autogen.sh is not my first priority right now, though, as

src/Makefile.am:9: warning: compiling 'main.c' with per-target flags requires 'AM_PROG_CC_C_O' in 'configure.ac'

Thanks for reminding me. That is necessary for older toolset, correct, and I have not actually checked my recent changes against the old tools in the Centos 7 mock chroot yet.

Aside from the warning, your branch builds and runs fine.

Brilliant!

Has anything changed that would effect make desktop-install ?

Not yet. I have not changed any install locations or the set of files to install to those locations. The only thing I have changed about the desktop-install rule is to remove an unnecessary trailing space character.

Until now.

There is still one relatively big thing which is not adressed yet with the installation, neither before nor (at this time) after my changes:

You can ./configure --prefix=/any/install/path to install at an arbitrary install location, but there are only a limited number of well-known places where the system looks for things like .desktop files or icons. Those well-known directories are usually based on XDG directories, and you usually have three of them: system-wide for system packages in /usr, system-wide for admin-installed packages in /usr/local, and local for non-privileged users in $HOME/.local.

So the question is what do you do when someone runs ./configure --prefix=PREFIX with PREFIX being neither /usr, /usr/local, or $HOME/.local.

I guess that is where something like make desktop-install comes in. However, it would also need to install some files into the appropriate well-known directory, not just call a few hook programs (some of which might have been replaced by the system watching the well-known directories where the files are installed to).

This is a part I have not touched yet, however that is on my list for this weekend.

KJ7LNW commented 1 year ago

So the question is what do you do when someone runs ./configure --prefix=PREFIX with PREFIX being neither /usr, /usr/local, or $HOME/.local.

I sort of think that it would be okay to have a special case for make desktop-install since it is a non-standard make target, as follows:

  1. If running as root, put it in /usr/local
  2. Otherwise ~/.local/

Somewhere the make install target tells the user "run make desktop-install for file associations" or something to that effect. The message could be modified to indicate where desktop-install will place files. It could also indicate that they can look at the PACKAGING file and do their own thing if they need more information.

Because they might (or might not) build as a local user and then sudo make install to install, the message that make install presents should describe what will happen if they make desktop-install as root vs non-root so they can decide.

ndim commented 1 year ago

I would change the logic a bit and move it to configure.ac, where it defines a few variables like maybe xdgrootdir and then defines some other variables like desktopdir or iconsdir relative to xdgrootdir:

BTW, the RPM spec file contains an *.appdata.xml file which may be useful inside files/ as well.

Also, perhaps part of the RPM spec %install section could be part of our make desktop-install.

As not having any of that does not change the current state of xnec2c, I would push these to after the big PR merge, though. That is becoming big enough as it is.

ndim commented 1 year ago

Status Update

These things MUST be addressed before merging this PR and cutting a release:

These things SHOULD be addressed before merging this PR and cutting a release:

These things COULD be addressed before merging this PR and cutting a release:

These things SHOULD be addressed in new PRs after merging this PR, before or after the next release:

It might be useful to merge all changes which change the way people build and install xnec2c at once, so people only need to adapt to build/install changes once, not every few days and every release for some time.

KJ7LNW commented 1 year ago

I would change the logic a bit and move it to configure.ac, where it defines a few variables like maybe xdgrootdir and then defines some other variables like desktopdir or iconsdir relative to xdgrootdir: [...]

  • Otherwise... abort the configure run and tell the user to use --with-xdg-rootdir

Good idea.

BTW, the RPM spec file contains an *.appdata.xml file which may be useful inside files/ as well.

I saw that too. It came from the original Fedora maintainer's .spec and I just copy-pasted his into the package with some minor tweaks. Popping that out into an XML is a good idea. Added #26 so that doesn't get lost.

Also, perhaps part of the RPM spec %install section could be part of our make desktop-install.

True. The desktop-install bits are hacked in place so adding make desktop-install would clean that up if it is compatible.

As not having any of that does not change the current state of xnec2c, I would push these to after the big PR merge, though. That is becoming big enough as it is.

Agreed, noted in #26.

These things MUST be addressed before merging this PR and cutting a release:

  • Adapt the documentation for other changes this cleanup may have made without me realizing.

I've been watching your commits and nothing particularly noteworthy stands out except that if we use make dist then the call to ./autogen.sh can change to ./autogen.sh # if building from git.

Can you think of anything else?

  • Figure out how to release xnec2c and document that.

What would it take to add github's CI integration (.github) and have make dist upload to a github "Release" .tar.gz file?

These things SHOULD be addressed before merging this PR and cutting a release:

  • Reduce or remove autogen.sh and adapt the documentation accordingly. The documentation being at least INSTALL, doc/xnec2c.html. xnec2c.spec.in will also need a change.

I think we should just replace the content of autogen.sh with exec autoreconf <whatever-is-best> and call it good. Then documentation doesn't need to change much.

  • Solve the issue of installing files to well-known directories outside of $(prefix).

I assume you're referring to make desktop-install and I like your --with-xdg-rootdir suggestion above. If you have other out-of-prefix concerns then please fill me in because I must have missed something.

These things COULD be addressed before merging this PR and cutting a release:

  • Silent make rules, which can make the "make" run look as follows but still allow seeing the used command line and flags when necessary.

Neat! Go for it if its easy.

These things SHOULD be addressed in new PRs after merging this PR, before or after the next release:

  • Translations. My wip-i18n branch has the basics figured out, but someone needs to make some non-technical decisions, and that someone is not me.

I think I'm that someone. :)

  • Convenience RPMs. My wip-rpms branch has make srpm make rpm and make mockbuild working.

Awesome!

It might be useful to merge all changes which change the way people build and install xnec2c at once, so people only need to adapt to build/install changes once, not every few days and every release for some time.

I agree. I'm planning a do this as part of a 4.5 release.

KJ7LNW commented 1 year ago

Hi @ndim ,

I've merged this branch into master. Your changes made it possible to build on OpenBSD. Thanks for your hard work on this!

If there are other changes you still wish to make for the buildsystem refactor/cleanup then feel free to open another PR.

-Eric, KJ7LNW