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

openfortivpn version 1.13.0 #586

Merged
merged 1 commit into from
Mar 22, 2020

Conversation

DimitriPapadopoulos
Copy link
Collaborator

No description provided.

@adrienverge
Copy link
Owner

Thanks Dimitri!

@adrienverge adrienverge merged commit 3fc2df1 into adrienverge:master Mar 22, 2020
@adrienverge
Copy link
Owner

I tried to create updates for Fedora and CentOS, but version 1.13.0 does not compile: https://kojipkgs.fedoraproject.org//work/tasks/9499/42689499/build.log

src/main.c: In function 'main':
src/main.c:76:24: error: expected ')' before 'RESOLVCONF_USAGE'
   76 | "                    " RESOLVCONF_USAGE "[--ca-file=<file>]\n" \
      |                        ^~~~~~~~~~~~~~~~
src/main.c:477:27: note: in expansion of macro 'usage'
  477 |    printf("%s%s%s%s%s%s", usage, summary, help_options,
      |                           ^~~~~
src/main.c:477:15: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
  477 |    printf("%s%s%s%s%s%s", usage, summary, help_options,
      |              ~^
      |               |
      |               char *
src/main.c:477:17: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
  477 |    printf("%s%s%s%s%s%s", usage, summary, help_options,
      |                ~^
      |                 |
      |                 char *
src/main.c:477:19: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
  477 |    printf("%s%s%s%s%s%s", usage, summary, help_options,
      |                  ~^
      |                   |
      |                   char *
src/main.c:477:21: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
  477 |    printf("%s%s%s%s%s%s", usage, summary, help_options,
      |                    ~^
      |                     |
      |                     char *
src/main.c:477:23: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
  477 |    printf("%s%s%s%s%s%s", usage, summary, help_options,
      |                      ~^
      |                       |
      |                       char *
src/main.c:76:24: error: expected ')' before 'RESOLVCONF_USAGE'
   76 | "                    " RESOLVCONF_USAGE "[--ca-file=<file>]\n" \
      |                        ^~~~~~~~~~~~~~~~
src/main.c:603:24: note: in expansion of macro 'usage'
  603 |  fprintf(stderr, "%s", usage);
      |                        ^~~~~
make: *** [Makefile:628: src/openfortivpn-main.o] Error 1

@DimitriPapadopoulos
Copy link
Collaborator Author

DimitriPapadopoulos commented Mar 22, 2020

Ah strange. I'll try on my Fedora 31 virtual machine and let you know.

@DimitriPapadopoulos
Copy link
Collaborator Author

It works for me on Fedora 31:

$ autoreconf -fi
$ 
$ ./autogen.sh
+ type autoconf
+ type automake
+ type aclocal
+ aclocal
+ autoconf
+ automake --add-missing
+ echo 'now you can run ./configure && make to build openfortivpn'
$ 
$ ./configure --prefix=/opt/openfortivpn
[...]
checking for resolvconf... /usr/sbin/resolvconf
configure: RESOLVCONF_PATH... /usr/sbin/resolvconf
configure: HAVE_RESOLVCONF... 1
checking for /usr/sbin/resolvconf... yes
configure: USE_RESOLVCONF... 0
checking that generated files are newer than configure... done
configure: creating ./config.status
config.status: creating Makefile
config.status: executing depfiles commands
config.status: executing timestamp commands
$ 
$ make
  CC       src/openfortivpn-config.o
  CC       src/openfortivpn-hdlc.o
  CC       src/openfortivpn-http.o
  CC       src/openfortivpn-io.o
  CC       src/openfortivpn-ipv4.o
  CC       src/openfortivpn-log.o
  CC       src/openfortivpn-tunnel.o
  CC       src/openfortivpn-main.o
  CC       src/openfortivpn-xml.o
  CC       src/openfortivpn-userinput.o
  CC       src/openfortivpn-openssl_hostname_validation.o
  CCLD     openfortivpn
$ 

I suspect you need to fully reinitialize Autoconf (autoreconf -fi) after Martin's changes.

@DimitriPapadopoulos DimitriPapadopoulos deleted the release branch March 22, 2020 10:26
@DimitriPapadopoulos
Copy link
Collaborator Author

@DimitriPapadopoulos
Copy link
Collaborator Author

The build machine is missing package systemd which provides resolvconf/resolvctl (see root.log). Strange, this package is installed by default on vanilla Fedora 31 installations - but perhaps Workstation only.

The result (see build.log]:

[...]
+ ./configure --build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu --program-prefix= --disable-dependency-tracking --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/var/lib --mandir=/usr/share/man --infodir=/usr/share/info
[...]
checking for resolvconf... DISABLED
configure: RESOLVCONF_PATH... DISABLED
configure: HAVE_RESOLVCONF... 0
checking for DISABLED... no
configure: USE_RESOLVCONF... 0
[...]

That's indeed a combination we haven't tested, at least recently. And it's good that you've caught it.

@DimitriPapadopoulos
Copy link
Collaborator Author

I think I can fix it. Do we move tag 1.13.0 or do we create tag 1.13.1?

@DimitriPapadopoulos
Copy link
Collaborator Author

I can simulate that on my Fedora machine without uninstalling systemd using:

sudo mv /usr/sbin/resolvconf /usr/sbin/resolvconf.bak

The result is identical:

$ ./configure --prefix=/opt/openfortivpn
[...]
checking for resolvconf... DISABLED
configure: RESOLVCONF_PATH... DISABLED
configure: HAVE_RESOLVCONF... 0
checking for DISABLED... no
configure: USE_RESOLVCONF... 0
[...]
$ 
$ make
[...]
  CC       src/openfortivpn-main.o
src/main.c: In function 'main':
src/main.c:76:24: error: expected ')' before 'RESOLVCONF_USAGE'
   76 | "                    " RESOLVCONF_USAGE "[--ca-file=<file>]\n" \
      |                        ^~~~~~~~~~~~~~~~
src/main.c:477:27: note: in expansion of macro 'usage'
  477 |    printf("%s%s%s%s%s%s", usage, summary, help_options,
      |                           ^~~~~
src/main.c:477:15: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
  477 |    printf("%s%s%s%s%s%s", usage, summary, help_options,
      |              ~^
      |               |
      |               char *
src/main.c:477:17: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
  477 |    printf("%s%s%s%s%s%s", usage, summary, help_options,
      |                ~^
      |                 |
      |                 char *
src/main.c:477:19: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
  477 |    printf("%s%s%s%s%s%s", usage, summary, help_options,
      |                  ~^
      |                   |
      |                   char *  CC       src/openfortivpn-main.o

src/main.c:477:21: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
  477 |    printf("%s%s%s%s%s%s", usage, summary, help_options,
      |                    ~^
      |                     |
      |                     char *
src/main.c:477:23: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
  477 |    printf("%s%s%s%s%s%s", usage, summary, help_options,
      |                      ~^
      |                       |
      |                       char *
src/main.c:76:24: error: expected ')' before 'RESOLVCONF_USAGE'
   76 | "                    " RESOLVCONF_USAGE "[--ca-file=<file>]\n" \
      |                        ^~~~~~~~~~~~~~~~
src/main.c:603:24: note: in expansion of macro 'usage'
  603 |  fprintf(stderr, "%s", usage);
      |                        ^~~~~
make: *** [Makefile:624: src/openfortivpn-main.o] Error 1
$ 

Note that a simple ./configure --disable-resolvconf has not the same effect:

$ sudo mv /usr/sbin/resolvconf.bak /usr/sbin/resolvconf
$ 
$ ./configure --prefix=/opt/openfortivpn --disable-resolvconf
[...]
checking for resolvconf... /usr/sbin/resolvconf
configure: RESOLVCONF_PATH... /usr/sbin/resolvconf
configure: HAVE_RESOLVCONF... 1
configure: USE_RESOLVCONF... 0
[...]
$ 

@mrbaseman I don't think we want that, do we?

@DimitriPapadopoulos
Copy link
Collaborator Author

DimitriPapadopoulos commented Mar 22, 2020

Fixed by pull request #587 and will work on Fedora / CentOS / EPEL, with resolvconf support entirely disabled.

These questions remain:

  • Do we want resolvconf support entirely disabled at build-time (HAVE_RESOLVCONF set to 0) or simply disabled by default at run-time (HAVE_RESOLVCONF set to 1 and USE_RESOLVCONF set to 0)?
  • If we want to keep resolvconf support at build-time but disable it by default at run-time, the spec files need to be modified to either install systemd as a build-time dependency but not as a run-time dependency, or alternatively add this Autoconf option to the build file:
    --with-resolvconf=/usr/sbin/resolvconf
  • I'm not entirely comfortable with these multiple options:
    --with-resolvconf=/path/to/resolvconf
    --with-resolvconf=DISABLED
    --disable-resolvconf
    At the very least I think --disable-resolvconf should entirely disable resolvconf support and be equivalent to --with-resolvconf=DISABLED which should be dropped.

@adrienverge
Copy link
Owner

@DimitriPapadopoulos thanks for investigating: it was indeed systemd requirement that was missing.

I've added it in the spec file, it builds on Koji now.

@adrienverge
Copy link
Owner

adrienverge commented Mar 22, 2020

@DimitriPapadopoulos, sorry, our messages crossed.

I think #587 makes sense. So I propose not to push "v1.13.0 + systemd fix" packages (already partly created on Koji for Fedora and CentOS), but instead wait for v1.13.1. What do you think?

EDIT: Strangely, its still fails for CentOS 7. Maybe systemd does not provide resolvconf? Anyway, v1.13.1 should fix it.

src/main.c: In function 'main':
src/main.c:76:24: error: expected ')' before 'RESOLVCONF_USAGE'
 "                    " RESOLVCONF_USAGE "[--ca-file=<file>]\n" \
                        ^
src/main.c:477:27: note: in expansion of macro 'usage'
    printf("%s%s%s%s%s%s", usage, summary, help_options,
                           ^
src/main.c:478:11: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
           PPPD_HELP, RESOLVCONF_HELP, help_config);
           ^
src/main.c:478:11: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
src/main.c:478:11: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
src/main.c:478:11: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
src/main.c:478:11: warning: format '%s' expects a matching 'char *' argument [-Wformat=]
src/main.c:76:24: error: expected ')' before 'RESOLVCONF_USAGE'
 "                    " RESOLVCONF_USAGE "[--ca-file=<file>]\n" \
                        ^
src/main.c:603:24: note: in expansion of macro 'usage'
  fprintf(stderr, "%s", usage);
                        ^
make: *** [src/openfortivpn-main.o] Error 1

@DimitriPapadopoulos
Copy link
Collaborator Author

I agree, let's wait for 1.13.1. It can be released pretty soon, I would just like some input from Martin on my suggestion to merge (or not) --with-resolvconf=DISABLED and --disable-resolvconf.

Are you able to add systemd as build-time but not run-time dependency?

@DimitriPapadopoulos
Copy link
Collaborator Author

Indeed CentOS 7 does not provide /usr/bin/resolvectl, let alone /usr/sbin/resolvconf:

CentOS 8, Fedora 30 and 31 do:

@adrienverge
Copy link
Owner

adrienverge commented Mar 23, 2020

Are you able to add systemd as build-time but not run-time dependency?

Yes I can. I added systemd as runtime requirement too, because I thought it was needed since at compile time we assume HAVE_RESOLVCONF. But maybe I'm wrong; do you think I should remove it?

Indeed CentOS 7 does not provide /usr/bin/resolvectl, let alone /usr/sbin/resolvconf:

Right; and no other package provides it (sudo yum provides '*/resolvconf').
I'm planning to revert BuildRequires: systemd (both for CentOS and Fedora) and simply package v1.13.1, which contains https://github.com/adrienverge/openfortivpn/pull/587/files. Does that sound good?

@DimitriPapadopoulos
Copy link
Collaborator Author

DimitriPapadopoulos commented Mar 23, 2020

But maybe I'm wrong; do you think I should remove it?

Absolutely, please remove systemd from run-time dependencies: openfortivpn calls resolvconf if it is available and otherwise falls back to updating /etc/resolv.conf directly as was the case before. Therefore systemd is completely optional. Actually moving /usr/sbin/resolvconf out of the way works around the 1.12.0 issue we're fixing with 1.13.

I'm planning to revert BuildRequires: systemd (both for CentOS and Fedora) and simply package v1.13.1, which contains https://github.com/adrienverge/openfortivpn/pull/587/files. Does that sound good?

Yes, you could even remove systemd from build-time dependencies. We would still have the fallback mechanism which updates /etc/resolv.conf directly and has been working for years. Besides resolvconf doesn't work (as expected at least) on CentOS/Fedora so we disable it anyway. We just disable it in different ways:

  • With systemd at build-time, we define HAVE_RESOLVCONF = 1 but USE_RESOLVCONF = 0 because we detect resolvconf doesn't work. Support for resolvconf is built-in, disabled by default and can be re-enabled at run-time with --use-resolvconf=1.
  • With systemd at build-time, we define HAVE_RESOLVCONF = 0 and USE_RESOLVCONF = 0. Support for resolvconf is not built-in and cannot be re-enabled at run-time with --use-resolvconf=1.

Actually I'm all in favour of disabling resolvconf support totally on CentOS/Fedora until we have answers to issue 1815605 on the Red Hat bug tracker and clarify how DNS parameters are supposed to be updated.

@mrbaseman
Copy link
Collaborator

mrbaseman commented Mar 23, 2020

the two configure options

--with-resolvconf=DISABLED and --disable-resolvconf
allow us to distinguish several scenarios:

  • disable resolvconf support completely (to avoid checks for the existence of a file that perhaps will never exist on some platforms)

  • compile in resolvconf support but disable it by default so that it can be enabled via config file or command line option when missing packages are installed and properly configured

  • enable resolvconf support completely and have it on by default

There are several options to simplify this and keep only one of the configure options:

  • make the decision at configure time and drop the case in which the user can have resolvonf support compiled in but disabled by default

  • always compile resolvconf support in and fall back to the most likely location if the executable is not found at configure time, maybe disable it by default if no working configuration is found

  • drop the switch that enables / disables the default for the runtime option and tie that decision to the magic that runs the tests at configure time, e.g. if a working resolvconf installation is detected, enable it by default, and do the same if a path is supplied by --with-resolvoconf, even if no executable is available is installed on the build-system.

Personally, I like the current implementation because for its flexibility, but I must admit that it's not really transparent. I'm fine, too, with each of the above changes. We just should be careful not to break things that we have just implemented for 1.13 and which are actively used anywhere.

Just a side note: I've just checked the case HAVE_RESOLVCONF = 0 and USE_RESOLVCONF = 1:
resolvconf support is disabled then, so USE_RESOLVCONF has no effect in this case.

@DimitriPapadopoulos
Copy link
Collaborator Author

@mrbaseman OK let's leave it as it is now. While I understand the purpose of these Autoconf options, I had this impression that there are too many of them and that we should perhaps discard this use case:

  • compile in resolvconf support but disable it by default so that it can be enabled via config file or command line option when missing packages are installed and properly configured

But as I said I'm fine with keeping these options, it's a good idea after all. It's just that the semantics are not clear at first sight:

--with-resolvconf=DISABLED
--disable-resolvconf

@adrienverge
Copy link
Owner

@DimitriPapadopoulos thanks for your explanations, it makes sense. I've removed systemd both from BuildRequires and Requires.

Packages for Fedora 31, Fedora 32 and CentOS 8 are built and on their way to publishing. For some reason, building for CentOS 7 fails currently (but it does not seem related to openfortivpn).

@mrbaseman
Copy link
Collaborator

I have improved the help strings of the configure options in #593

@adrienverge
Copy link
Owner

For some reason, building for CentOS 7 fails currently (but it does not seem related to openfortivpn).

This morning, build passed. I pushed the update to CentOS 7, it should arrive to stable in 14 days.

@cognifloyd
Copy link

@adrienverge I'm very sad that the CentOS 7/8 builds of openfortivpn don't seem to be using the systemd libs. Without that Type=notify systemd units aren't possible. How hard would it be to add those libs to the build so that sd_notfiy works? (notify support added in #370)

@DimitriPapadopoulos
Copy link
Collaborator Author

DimitriPapadopoulos commented Apr 2, 2020

@cognifloyd Hi Jacob, this is indeed a side effect of removing systemd from the specifications because systemd ships with a broken resolvconf executable on Fedora. Unfortunately systemd also serves another purpose: notifications.

@adrienverge There are two ways of disabling resolvconf support:

  1. removing systemd altogether at build-time, at the expense of supporting systemd notifications too,
  2. adding --with-resolvconf=DISABLED --disable-resolvconf to the spec file for as long as we don't support resolvectl in addition to resolvconf (see Improve DNS handling? #600).

Without changing the oopenfortivpn sources, it might be possible to updates the spec file by inserting back BuildRequires and Requires and adding --with-resolvconf=DISABLED --disable-resolvconf to the Autoconf options.

@adrienverge
Copy link
Owner

Without changing the oopenfortivpn sources, it might be possible to updates the spec file by inserting back BuildRequires and Requires and adding --with-resolvconf=DISABLED --disable-resolvconf to the Autoconf options.

Thanks @DimitriPapadopoulos 👍
Restoring requirements and adding --with-resolvconf=DISABLED --disable-resolvconf is doable, but would it have other implications? I mean could it break behavior for some Fedora/CentOS users with resolvconf?

@cognifloyd do you want to propose a diff for the spec file?

@DimitriPapadopoulos
Copy link
Collaborator Author

There should be no implications because --with-resolvconf=DISABLED --disable-resolvconf disables support for resolvconf (broken on CentOS/Fedora) at build-time. The CentOS/Fedora behaviour would remain unchanged: openfortivpn falls back on modifying /etc/resolv.cong directly, the method that had been used for ever by openfortivpn and FortiClient.

@DimitriPapadopoulos
Copy link
Collaborator Author

@cognifloyd The spec file is maintained here:
https://src.fedoraproject.org/rpms/openfortivpn

@cognifloyd
Copy link

cognifloyd commented Apr 6, 2020

I'm sorry, I am unable to install fedpkg to be able to work with the repos on src.fedoraproject.org as my primary development machine is gentoo. I've run out of time to figure out how to get a setup for editing rpms. If I get time another day, I can try again. Is there any chance you will have time to update the rpm?
Thanks!

@adrienverge
Copy link
Owner

@cognifloyd can you try this build: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2020-19aca4f76e and report if it works for you?

@cognifloyd
Copy link

Woohoo. That worked on both el7 and el8.
I can't actually test anything with changing resolvconf because the fortigate I'm connecting to is not configured to send any dns details, so there's nothing to change.

@adrienverge
Copy link
Owner

Cool! Thanks for +1 karma on Bodhi 👍

@cognifloyd
Copy link

This is rather odd to be using a closed PR for communication. Sorry.
@adrienverge could you push the EL7 rpm to stable? I don't understand why bodhi didn't do it automatically after 14 days.
https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2020-19aca4f76e

@mrbaseman
Copy link
Collaborator

mrbaseman commented Apr 24, 2020

This is rather odd to be using a closed PR for communication. Sorry.

this is because the communication has started on a pull request which has been merged.

I admit we should have moved it to a new issue. We have done so for some aspects, but not for the general discussion about packaging. We should keep this in mind for future releases and first open a github issue. In the github issue about tagging a new release we can

  • start the discussion what we want to merge before finalizing the release
  • link the PR that increments the version number and updates the CHANGELOG
  • discuss all the questions about packaging
  • discuss possible problems and bugs to fix that arise during that process on particular platforms

For the paggageing of 1.13.0 it's probably too late, but:
Anyone who would answer in this thread, please consider opening a new issue if further problems arise around the 1.13.0 release.

@adrienverge
Copy link
Owner

I agree with @mrbaseman.

@cognifloyd I pushed the update to stable on Bodhi. Next time, only one ping on Bodhi should be enough ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants