Refactor of RPM packaging files + migrate from sysvinit -> systemd service #97
Conversation
gs-kamnas
force-pushed
the
rpm_refactor
branch
from
73f622c to
efa1fe4
Compare
|
Hello @gs-kamnas, can you better explain this PR? |
@fralken sure; my overall goals in this change are to use a native systemd service for managing cNTLM when run as a Linux system service, given that most major Linux distros are using systemd as the initsystem as well as simplify the RPM specfile and associated build process via the usage of macros present in recent versions of RPM. Furthermore, I opted to amend the CI based workflow to better test the end result by building and installing the RPM package in both a RHEL as well as SuSE based container environment. |
|
That's interesting! Have you checked the option to run a job within a container? Maybe it's possible to avoid maintaining the dockerfiles by implementing specific jobs (that is, by splitting the "build" job in the c-cpp.yml file in separate jobs depending on different linux distros) |
This looks like it will work however will of course bind the RPM build task more tightly to github actions as opposed to being able to run it standalone. If this is preferable I can certainly refactor to the above. |
This is a valid point, but when is the need to build the rpm locally not running on a RHEL or Suse host? |
Very niche case agreed, therefore refactored to use Github actions as requested as that will likely be a bit easier to maintain long term. |
|
hello @gs-kamnas and @jschwartzenberg . Is it ready to merge this PR? @gs-kamnas are you able to verify also the debian package? |
Would prefer to close out the systemd user vs. system scoped but parameterized with username discussion 1st; then I can absolutely verify that the Debian package functions as well. Would you prefer I do so against any particular version of Debian, or is the latest OK? |
Of course, first complete this PR. I never investigated too much on rpm and deb packages because I'm using cntlm mainly on windows and mac. |
|
Can you also rebase this PR on the latest master branch? |
Rebased as well as refactored as per @jschwartzenberg 's feedback, Therefore will get on testing this comprehensively under the latest versions of RHEL, SLES as well as Debian. Afterwards, I believe we are good to consider merging. |
|
Hello, I see that the
Is there a way to make |
Definitely should be doable, therefore will look into making this more consistent (and as close as possible to the RHEL behavior) |
|
@fralken It appears to be rather straightforward to disable the CFLAGS, LDFLAGS and related environment variable overrides by setting Furthermore, debug symbols extraction is an opt-in feature in the SLES RPM macro implementation (that I now conditionally enable if on SLES). Therefore, I now believe the behavior with regards to the build flags used and binaries generated to be far more consistent across distros (which has the added benefit of also remediating the build failure on Fedora). Finally, I have added a verification step in the GH actions pipeline to assert that the installed binaries are stripped, as well as log the size of such. |
This is great! thanks a lot! I was testing with fedora but I don't know if there are any other major linux distributions that use RPM to test upon. Also, If we release the packages, which rpm build should be released? |
|
In my opinion what is still missing is clean up the repo a bit.
Finally, it would be nice, for better documenting, to squash intermediate commits and leave the relevant ones. And also rebase on master. |
|
@fralken Agree with regards to recommended changes, therefore will implement over next several days, |
gs-kamnas
force-pushed
the
rpm_refactor
branch
2 times, most recently
from
3860c28 to
eee5730
Compare
|
|
Clean / distclean target in makefile should now be working as desired. Also, I believe I have handled all cases where the build was introducing modifications to the source tree. Rebase is still pending as I may also want to review some of the written documentation for accuracy as well. |
|
Hello @gs-kamnas are you planning to complete this PR? It is almost there, just a small refactoring is needed. |
|
Apologies for the radio silence, too many competing priorities therefore I haven't had the time to dedicate to this for a while. Will aim to address the documentation items that are outstanding as well as rebase this coming week. |
gs-kamnas
force-pushed
the
rpm_refactor
branch
from
eee5730 to
69d6268
Compare
|
Rebased onto latest master therefore reviewing documentation |
|
@fralken I consider this ready for a re-review when you have the time. Thanks in advance! |
|
Hello @gs-kamnas, thanks for the update. I see that there are still 3 (out of 4) pending reviews in the |
|
Acknowledged with regards to squashing down the number of commits to a more reasonable/relevant number. However, I cannot see your comments on the Makefile @fralken ; are you certain you have submitted your review? Screenshot: |
gs-kamnas
force-pushed
the
rpm_refactor
branch
3 times, most recently
from
6b577f3 to
e755b93
Compare
… the package metadata
Add validation to ensure that builds do not make persistent changes to the source tree.
…e management * Relocate all packaging files under linux/<distro> * Use new specfile derived from the Fedora EPEL upstream * Build with debugging symbols at all times, such that we can strip them into the associated devel/debuginfo package during packaging. * Updates to README as needed * Use native systemd units for service management This commit implements both a user scoped systemd unit in order to support use cases where each user maintains one's own configuration file, running cNTLM as one's own user, as well as the traditional system scoped unit that is prevalent in most distribution packages. This is more compatible (as it does not require user scoped systemd instances) however less flexible. Additionally, the behavior of both systemd units is designed to pass parameters specified in the OPTARGS parameter in /etc/sysconfig/cntlmd to both user as well as system instances of cntlm, for purposes of centralized management of configuration parameters, such as the proxy hostnames or PAC file, while still allowing users to maintain their credentials in a configuration file private to their homedir.
gs-kamnas
force-pushed
the
rpm_refactor
branch
from
e755b93 to
983c7d0
Compare
|
| @@ -6,6 +6,8 @@ DESTDIR := | |||
| PREFIX := /usr/local | |||
| SYSCONFDIR := $(DESTDIR)/etc | |||
| BINDIR := $(DESTDIR)$(PREFIX)/sbin | |||
| INST_BINDIR := $(PREFIX)/sbin | |||
There was a problem hiding this comment.
Here I think you can use BINDIR, since DESTDIR is typically empty in real scenario and I guess it set only for testing purposes.
| install -M 644 -f $(MANDIR)/man1 doc/$(NAME).1; \ | ||
| install -M 600 -c $(SYSCONFDIR) doc/$(NAME).conf; \ | ||
| elif [ "`uname -s`" = "Darwin" ]; then \ | ||
| install -d $(BINDIR)/; \ | ||
| install -m 755 -s $(NAME) $(BINDIR)/$(NAME); \ | ||
| install -m 755 $(STRIP) $(NAME) $(BINDIR)/$(NAME); \ | ||
| install -d $(MANDIR)/man1/; \ | ||
| install -m 644 doc/$(NAME).1 $(MANDIR)/man1/$(NAME).1; \ | ||
| [ -f $(SYSCONFDIR)/$(NAME).conf -o -z "$(SYSCONFDIR)" ] \ | ||
| || install -d $(SYSCONFDIR)/; \ | ||
| install -m 600 doc/$(NAME).conf $(SYSCONFDIR)/$(NAME).conf; \ | ||
| else \ |
There was a problem hiding this comment.
here it is better to write elif [ "`uname -s`" = "Linux" ]; then \ because we don't want it to be executed on Windows
| install -D -m 755 -s $(NAME) $(BINDIR)/$(NAME); \ | ||
| install -D -m 755 $(STRIP) $(NAME) $(BINDIR)/$(NAME); \ | ||
| sed "s#%BINDIR%#$(INST_BINDIR)#g" cntlm-user.in > cntlm-user; \ | ||
| install -D -m 755 $(NAME)-user $(LIBEXECDIR)/$(NAME)-user; \ | ||
| install -D -m 644 doc/$(NAME).1 $(MANDIR)/man1/$(NAME).1; \ | ||
| [ -f $(SYSCONFDIR)/$(NAME).conf -o -z "$(SYSCONFDIR)" ] \ | ||
| || install -D -m 600 doc/$(NAME).conf $(SYSCONFDIR)/$(NAME).conf; \ |
There was a problem hiding this comment.
Finally, line 164 below can be something like:
[ -f $(SYSCONFDIR)/$(NAME).conf ] && @echo; echo "Cntlm will look for configuration in $(SYSCONFDIR)/$(NAME).conf"
So that it is displayed only when the installation is successful
I'm sorry, my fault, I forgot to press "submit review". |
This PR introduces the following changes: