ppp icon indicating copy to clipboard operation
ppp copied to clipboard

Preparation for release 2.4.10

Open mjeveritt opened this issue 4 years ago • 7 comments

Per https://github.com/paulusmack/ppp/issues/111#issuecomment-757405517 , let's start discussion on any issues arising from release 2.4.9 and patches we'd like to integrate into the Next release (expected 2.4.10).

I shall be PR'ing patches shortly from Gentoo per @Polynomial-C 's request.

mjeveritt avatar Jan 15 '21 04:01 mjeveritt

Just a suggestion to make releases a bit more frequently :) As still flow of the patches is relatively high it would better to allow ebvaluate accepted commits in smaller chunks to allow easier identify some changes. I think that new release after ot more than 50 commits would be kind of optimum for ppp.

kloczek avatar Feb 06 '21 14:02 kloczek

I agree. Even the replies / pull requests are very slow. This tool is very useful almost in any route deploying if you need to build a VPN / AC router. I found recently issues in slprintf and strlcpy functions. I know that are some improved functions of snprinf and strncpy, but I think they are outdated. slprinf is not processing correctly the formats. Instead replacing the arguments is displaying the format instead, example:

//  slprintf(buf, sizeof(buf), "%08lX%04X%02hX",
  snprintf(buf, sizeof(buf), "%08lX%04X%02hX",
           (unsigned long int) time (NULL),
           (unsigned int) getpid (),
           cnt & 0xFF);

slprintf is putting in buf value "%lX60cb2c01%hX". snprintf on the other hand is putting in buf this value: "60CB2E2F2459F900".

I don't know the reasons why these functions where replicated, but I think we can rewrite them using standard C lib funtions and to behave same as actual sl.. functions.

Opinions?

EasyNetDev avatar Jun 17 '21 12:06 EasyNetDev

Hi there, I'm from Gentoo musl. Please take a look on https://github.com/ppp-project/ppp/pull/293 too, more info inside pr

ghost avatar Jul 09 '21 12:07 ghost

As to why we have slprintf still, the main reason is that it has some useful formats that the standard C *printf functions don't have, for example, %I (IP address), %q (quoted string), %v ("visible" string, i.e. non-printing chars translated to a printing form), %t (time and date), %B (print buffer in hex), %P (print PPP packet).

You're right it didn't have %X, but that is fixed now.

paulusmack avatar Dec 19 '21 23:12 paulusmack

I think the next release should be called 2.5.0, given the major change to the build system. I would like to do it in early January 2022.

paulusmack avatar Dec 19 '21 23:12 paulusmack

@paulusmack: Thanks for last merging :)

There was a problem with PR of @EasyNetDev, maybe good to look commits?

  • https://github.com/EasyNetDev/ppp/commits?author=EasyNetDev

Have you seen @enaess comments here:

  • https://github.com/ppp-project/ppp/issues/111#issuecomment-944852056
  • https://github.com/ppp-project/ppp/issues/111#issuecomment-999082357
  • https://github.com/ppp-project/ppp/issues/111#issuecomment-1007170620

About OpenSSL 3.0.0: Have you remarks?

  • https://github.com/ppp-project/ppp/issues/242

Neustradamus avatar Jan 13 '22 08:01 Neustradamus

@Neustradamus There is a bunch of issues that either can be closed or triaged. You able to go through the set of issues and labeling them a:bug or a:feature-request or a:must-fix, or similar schema?

enaess avatar Aug 16 '22 21:08 enaess

I think last PRs before the new build will be:

  • https://github.com/ppp-project/ppp/pull/379

Neustradamus avatar Dec 27 '22 01:12 Neustradamus

About a 2.5.0 build:

  • Debian 12 soon: @bootc
  • Fedora 38 soon: @yarda, @msekletar, @sahanaprasad07, @szpak, @tstellar, @tbaederr, @besser82, @ignatenkobrain

Hope it will be good!

Neustradamus avatar Jan 04 '23 03:01 Neustradamus

I have prepared a tentative updated Debian package with a current snapshot: https://salsa.debian.org/debian/ppp/-/commits/master-md , while also removing most patches. No significant issues, and the new autoconf-based build infrastructure is working very well even if I had to use different hacks: we could use, at least, switches for disabling crypt(3) and selecting which plugins should be built.

We will not be able to target the next Debian release (12) because a library transitions freeze is scheduled to happen in one week, but I may make backports later if needed. Hopefully an official public API will make this much easier in the future.

rfc1036 avatar Jan 07 '23 19:01 rfc1036

Thanks for all that work @rfc1036!

I think @bootc similarly concluded the timeframe to get pppd 2.5.0 into bookworm (12) release would be near impossible in an email to @Neustradamus and @paulusmack and myself. Packaging it for a Bookworm release should perhaps started 6 months ago together with the proposed header-file re-organization, and deal with the breakage in existing packages.

Regardless, I think 2.5.0 with the latest set of patches would be helpful when going forward; so we should proceed with a release despite it being too late for Debian. If we leave a bit of room to take your suggestions e.g. stuff you need to patch perhaps should go upstream and directly to pppd? Also, with pkg-config, does the need for dh_ppp script go away entirely as sister packages should just use that instead of trying to infer the version in any other way? Any other variables you think would be helpful to the pkg-config here?

Did you also get to produce a stripped down ppp udev variant? Or do you need maybe another configure option to leave out crypt.h and friends? Any other features you see lacking from ./configure?

enaess avatar Jan 07 '23 21:01 enaess

We could probably have done it with a couple more weeks, but it is how it is.

I am waiting for @bootc's review to upload that snapshot to Debian/experimental: there is no need to wait for a release.

After that (or even right now, if it were already merged), I would be happy to get a new snapshot with the reorganized headers setup, to really test how that works. I am not sure how much of dh_ppp and the symbols file machinery will still be needed, since I have not looked at the patch. The best case scenario would be to have the ppp package provide a ppp-api-TIMESTAMP virtual package that plugin packages could depend on, but I think that we will still need dh_ppp to automatically provide the right name. pkgconfig does not help distributions because we already know where the headers are (actually: in the default path) but we need something special to manage the API version since this is not about linking a program with a library.

For the udeb package I am disabling crypt(3), /etc/shadow support and some plugins by editing config.h and the generated makefiles, and for the time being that's enough. Just adding configure options to disable crypt(3) to and list the desired plugins should be trivial, but I am not very good at autoconf.

I would appreciate libtool not installing the useless .a and .la files, but we can still delete them later.

rfc1036 avatar Jan 07 '23 23:01 rfc1036

I am waiting for @bootc's review to upload that snapshot to Debian/experimental: there is no need to wait for a release.

Thanks for all the work on that @rfc1036; there are a couple of tweaks I'd like to do before uploading, mostly related to the symbol versioning machinery, and we should probably use a 2.5.0~ revision rather than 2.4.10~ given the discussion in this thread. I'll sort that out.

After that (or even right now, if it were already merged), I would be happy to get a new snapshot with the reorganized headers setup, to really test how that works. I am not sure how much of dh_ppp and the symbols file machinery will still be needed, since I have not looked at the patch. The best case scenario would be to have the ppp package provide a ppp-api-TIMESTAMP virtual package that plugin packages could depend on, but I think that we will still need dh_ppp to automatically provide the right name. pkgconfig does not help distributions because we already know where the headers are (actually: in the default path) but we need something special to manage the API version since this is not about linking a program with a library.

The reason we don't use a virtual package is that there are packages that don't strictly depend on ppp, just list it inRecommends even though they ship a plugin file. We don't want to make ppp a hard dependency in this case but they still need a way of saying they are only compatible with a certain range of versions of ppp. Hence the mechanism I came up with. The pkg-config script may significantly simplify dh_ppp but it won't remove the need for it entirely.

For the udeb package I am disabling crypt(3), /etc/shadow support and some plugins by editing config.h and the generated makefiles, and for the time being that's enough. Just adding configure options to disable crypt(3) to and list the desired plugins should be trivial, but I am not very good at autoconf.

Does the installer's libc still not have crypt() support? I'll ask the installer team and double-check.

I would appreciate libtool not installing the useless .a and .la files, but we can still delete them later.

That's easily fixed either on our end or upstream; I'll propose a PR for it.

bootc avatar Jan 08 '23 12:01 bootc

Does the installer's libc still not have crypt() support? I'll ask the installer team and double-check.

These days we have the separate libxcrypt for crypt(3) and this has a libcrypt1-udeb package. I don't see why we should go out of our way to disable this support these days. @rfc1036?

bootc avatar Jan 08 '23 14:01 bootc

Even if I am the libxcrypt maintainer I am not sure of how many udeb packages actually use it. I suspect that at this point it is always pulled in, because it is needed to hash the passwords with the modern algorithm.

rfc1036 avatar Jan 08 '23 15:01 rfc1036

Cool!

Sounds like libxcrypt may solve that problem for us without having to do anything. Is there anything you found in packaging pppd, @rfc1036, that could be helpful for the project.

  • switch from libdir to datastoredir for installing (pkgconfig)
  • pathnames, connect-errors becomes ppp-connect-errors
  • others?

enaess avatar Jan 08 '23 16:01 enaess

Right now we have to maintain compatibility with the existing paths which have always been used by Debian, but if we could move/rename these files then they should be moved to /run/pppd/ and /var/log/pppd/. Except that Red Hat already uses /run/ppp/ and /var/log/ppp/ so we should follow them even if it is slightly suboptimal. The same applies to the TDB file.

rfc1036 avatar Jan 08 '23 16:01 rfc1036

@rfc1036 for /run/pppd and /var/log/pppd, isn't the --with-runtime-dir= and --with-logfile-dir= sufficient overrides for configure in this case?

enaess avatar Jan 08 '23 22:01 enaess

@rfc1036

I would appreciate libtool not installing the useless .a and .la files

Is there a good way around it? if so I haven't yet found it. For the debian packages I maintain, I still had to modify my pkg recipe to nuke the files post "make install". Please enlighten me if you know how ..

enaess avatar Jan 08 '23 22:01 enaess

Of course, my point was that I do not recommend changing the defaults to the Debian paths. If they can be changed then at this point I suggest using the Red Hat ones.

I am not a libtool expert, but I understand that it is possible to stop building the static plugins with AC_ENABLE_STATIC(no). There is no way to persuade libtool to not install the .la files.

rfc1036 avatar Jan 08 '23 22:01 rfc1036

@rfc1036 I tested the AC_ENABLE_STATIC(no) option, sure that doesn't produce the .a files. I think it is the equivalent of specifying --disable-static

Still, the .la files are being produced. They seem to be associated with the .so files installs (details about shared objects inside them).

enaess avatar Jan 10 '23 17:01 enaess

Yes, nothing can be done about the .la files, so distributions have to remove them manually. I will not get into the details, but the libtool maintainers feel really passionate about this...

rfc1036 avatar Jan 10 '23 17:01 rfc1036

Fair. I had to deal with this for my own Debian packages. I think it is a known / documented step you have to do to work around this.

enaess avatar Jan 10 '23 17:01 enaess

Yes, nothing can be done about the .la files, so distributions have to remove them manually. I will not get into the details, but the libtool maintainers feel really passionate about this...

I can vouch that Gentoo Linux has a function in the build/deployment system that strips them out before installing.

mjeveritt avatar Jan 10 '23 19:01 mjeveritt

Yes, nothing can be done about the .la files, so distributions have to remove them manually. I will not get into the details, but the libtool maintainers feel really passionate about this...

I can vouch that Gentoo Linux has a function in the build/deployment system that strips them out before installing.

I's merely a find ... -type f -name '*.la' -delete call (which has to be placed in each and every ebuild) and not a specific function.

Polynomial-C avatar Jan 10 '23 22:01 Polynomial-C

Yes, nothing can be done about the .la files, so distributions have to remove them manually. I will not get into the details, but the libtool maintainers feel really passionate about this...

I can vouch that Gentoo Linux has a function in the build/deployment system that strips them out before installing.

I's merely a find ... -type f -name '*.la' -delete call (which has to be placed in each and every ebuild) and not a specific function.

Derp my bad, we lost the built-in when that eclass got killed off ... :roll_eyes:

Also, Happy New Year Lars :stuck_out_tongue:

mjeveritt avatar Jan 11 '23 09:01 mjeveritt

Some informations here:

  • https://www.linuxfromscratch.org/blfs/view/svn/introduction/la-files.html

Neustradamus avatar Jan 11 '23 18:01 Neustradamus

Thanks @Neustradamus !

I believe Debian using similar construct in their rules/control files:

find ... -type f -name '*.la' -delete

Despite the annoyance, pppd doesn't use lt_load() function so this workaround is safe.

enaess avatar Jan 11 '23 18:01 enaess

Hi, I tried Fedora scratch build from the HEAD and LGTM: https://koji.fedoraproject.org/koji/taskinfo?taskID=96019866

One minor thing: you should probably add the copy of the licenses. E.g. AFAIK GPL requires the copy of the license to be included in the package/release.

yarda avatar Jan 11 '23 19:01 yarda

@yarda - Could you please spell out what you mean by this, e.g. include a LICENSE.BSD-3WAY file or similar.

Also, given the different licenses floating around in the source code for pppd, it could serve as a good reason to break some of these "sub-modules" into separate projects under the ppp-project umbrella. For example, I'd love to see the pppoe plugin, pppoatm, and radius plugins be relocated in a different git repository with respective configure, CI builds, and correct licenses spelled out, and it could also make pppd easier to manage.

enaess avatar Jan 11 '23 20:01 enaess