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

Use Alien::cmark to find or build libcmark #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Grinnz
Copy link

@Grinnz Grinnz commented Apr 1, 2019

The Alien::Build system allows creating modules that provide an interface to link against the system version of a library if appropriate, or otherwise download and install the library into Perl sharedirs (share build).

I've released Alien::cmark which can provide libcmark in this way, so it could be used by this module instead of relying on the user to have installed libcmark and its headers, thus making cpan CommonMark "just work" for most users (provided they have a C compiler). It uses pkg-config to determine if there is an appropriate system version to use.

Currently, Alien::cmark requires a minimum libcmark version of 0.21.0 as does this module, but I included a version check for the generated Makefile.PL here as well (possible since Alien::Base 1.55). The share build will install the latest version as of the Alien::cmark release.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.506% when pulling f04acb8 on Grinnz:use_alien_cmark into 27e85ba on nwellnhof:master.

@nwellnhof
Copy link
Owner

Looks nice but I'd like to retain the feature to use a custom version of libcmark installed in a custom directory. This is currently done by passing arguments INC and LIBS to Makefile.PL or by setting PERL_MM_OPT. This is useful for development or when testing with older libcmark versions on Travis (see the failing tests). Can this be done with Alien::cmark?

@Grinnz
Copy link
Author

Grinnz commented Apr 5, 2019

I'm not sure if there's a specific way for Alien::Build to use it, but if pkg-config and PATH can be set up to indicate the custom location then Alien::cmark will use it.

@nwellnhof
Copy link
Owner

Also, what happens if pkg-config doesn't find libcmark? Does Alien::cmark install libcmark in /usr/local by default? Since cmark is now packaged by modern distros, users should preferably be instructed to install the libcmark-dev package. (I also have to update the documentation in this regard.) I wouldn't want to install a custom libcmark build without explicit user approval if there's a maintained distro package available. I can see the benefit for CPAN Testers, though.

@Grinnz
Copy link
Author

Grinnz commented Apr 5, 2019

It installs it in the Perl share directories, which in practice is somewhere in @INC. Advising users to install libcmark-dev or equivalent is a good idea, but a lot of users will not read/understand this step and this is unlikely to be a package most users have already, and so the Alien::Build method allows CommonMark to be installable in one step for these users. (as well as cpantesters as you noted)

@Grinnz
Copy link
Author

Grinnz commented Apr 5, 2019

One way you could get Alien::Build to use a custom system library mentioned by @plicease would be env PKG_CONFIG_PATH=/custom/path/lib/pkgconf cpanm Alien::cmark. You would also want to add the custom installation's bin directory to PATH since Alien::cmark will look for the executable as well (since it provides both). He also mentioned that env PKG_CONFIG_PATH=/custom/path/lib/pkgconf af install --class Alien::cmark would adjust the config to a different system install without reinstalling Alien::cmark (using App::af).

If you specifically want to support the current method of supplying args to EUMM, then an alternative is keeping the current method of linking to the system library but using Alien::cmark as a fallback. This is a lot more complex but doable.

@nwellnhof
Copy link
Owner

One way you could get Alien::Build to use a custom system library mentioned by @plicease would be env PKG_CONFIG_PATH=/custom/path/lib/pkgconf cpanm Alien::cmark.

How would this work on non-Linux systems where pkg-config typically isn't available?

I'm also concerned about security updates to libcmark. In many cases, libcmark is used to process untrusted Markdown over the network and security issues in a C library can have severe consequences. Having a vulnerable library somewhere in @INC will likely go undetected and it's not clear to me how the update procedure to a newer libcmark version would look like. It seems that the libcmark version is hardcoded in Alien::cmark.

I'd much prefer a solution that only falls back to Alien::cmark on explicit user request or if the AUTOMATED_TESTING environment variable is set. But as you said, this is a lot more complex.

@plicease
Copy link

plicease commented Apr 8, 2019

One way you could get Alien::Build to use a custom system library mentioned by @plicease would be env PKG_CONFIG_PATH=/custom/path/lib/pkgconf cpanm Alien::cmark.

How would this work on non-Linux systems where pkg-config typically isn't available?

Alien::cmark (via Alien::Build) uses PkgConfig.pm (https://metacpan.org/pod/PkgConfig) if the command line pkg-config is not available. On Strawberry Perl this is actually the "system" pkg-config.

I'm also concerned about security updates to libcmark. In many cases, libcmark is used to process untrusted Markdown over the network and security issues in a C library can have severe consequences. Having a vulnerable library somewhere in @INC will likely go undetected and it's not clear to me how the update procedure to a newer libcmark version would look like. It seems that the libcmark version is hardcoded in Alien::cmark.

There is a lot to unpack here, but I don't see how the security concerns to installing software from a known location over https from github are significantly greater than downloading modules from CPAN.

Upgrades to Alien::cmark can be done either by re-installing the module, or by using af install -c cmark. This is a manual process, but again, it doesn't seem to be any more complicated than using INC, LIBS and PERL_MM_OPTS to use a cmark from a different location in the event that you don't have a system cmark to use. In the case of a system cmark the system's package manager is responsible for upgrades.

Aliens generally download the latest stable version of a package, but sometimes there are good reasons to download a fixed version. If the version does need to be fixed in the alien, then the Alien can be updated and released to match the appropriate version. In the few cases where this is necessary I haven't found it overly onerous for committed maintainers. A protection against this is to have a few maintainers share co-maint in the event that one is not available.

I'd much prefer a solution that only falls back to Alien::cmark on explicit user request or if the AUTOMATED_TESTING environment variable is set. But as you said, this is a lot more complex.

Requiring explicit user request does sort of defeat the purpose of making it easy to install out of the box, but I can see if it were appropriately documented it would be at least easier.

@plicease
Copy link

plicease commented Apr 8, 2019

I'd much prefer a solution that only falls back to Alien::cmark on explicit user request or if the AUTOMATED_TESTING environment variable is set. But as you said, this is a lot more complex.

Requiring explicit user request does sort of defeat the purpose of making it easy to install out of the box, but I can see if it were appropriately documented it would be at least easier.

Incidentally, although the default is to attempt a share install, Alien::Build based Aliens DO already have a standard controls for forcing a system or share install using the ALIEN_INSTALL_TYPE environment variable. For users who do not want to ever download non-CPAN packages over the network and rely only on system libraries they can set ALIEN_INSTALL_TYPE=system. I recommend this for operating system vendors building packages for Alienized CPAN modules.

@nwellnhof
Copy link
Owner

Alien::cmark (via Alien::Build) uses PkgConfig.pm (https://metacpan.org/pod/PkgConfig) if the command line pkg-config is not available.

Apparently only if PkgConfig.pm is installed. Even if PkgConfig.pm fully supported Windows with its various flavors, MacOS, and whatnot, this would add another dependency.

I don't see how the security concerns to installing software from a known location over https from github are significantly greater than downloading modules from CPAN.

I'm not talking about the source download but what happens if a security issue in libcmark is discovered and the library has to be upgraded to a newer version.

Upgrades to Alien::cmark can be done either by re-installing the module, or by using af install -c cmark. This is a manual process, but again, it doesn't seem to be any more complicated than using INC, LIBS and PERL_MM_OPTS to use a cmark from a different location in the event that you don't have a system cmark to use.

But it's unlikely that users know about this procedure (or ALIEN_INSTALL_TYPE), and many even won't be aware of the libcmark binary that was downloaded and installed behind their backs. If users are forced to install libcmark manually, they at least know about the dependency.

Aliens generally download the latest stable version of a package, but sometimes there are good reasons to download a fixed version.

For Alien::cmark, the version seems to be hardcoded, maybe because there's simply no way to discover the latest GitHub release. Anyway, the libcmark API isn't stable yet and automatically downloading the latest release isn't a good idea as it could suddenly break things. But there should be an easy way for users to install or upgrade to a specific libcmark version on all platforms.

Requiring explicit user request does sort of defeat the purpose of making it easy to install out of the box.

Yes, but for now, I'd prefer a bit of inconvenience over the issues mentioned above.

@plicease
Copy link

plicease commented Apr 8, 2019

Apparently only if PkgConfig.pm is installed.

PkgConfig.pm is dynamically added as a prereq iff pkg-config is not found.

Even if PkgConfig.pm fully supported Windows with its various flavors, MacOS, and whatnot, this would add another dependency.

PkgConfig.pm is only a prereq if pkg-config is not found.

It is also possible to use multiple detection strategies in a single alienfile. Alien::Libxml2 for example uses either pkg-config or ExtUtils::CBuilder depending on the first one that works. I find that the latter has been more reliable that Devel::CheckLib. Honestly, I think for this it is overkill, but that is certainly an option if you have grave concerns about pkg-config.

I also find that pkg-config is in practice quite reliable and the easiest to override.

I'm not talking about the source download but what happens if a security issue in libcmark is discovered and the library has to be upgraded to a newer version.

Well, if you are using the system cmark then you should get the system updates through the normal update process. If you happen to have used the version downloaded from source, as mentioned this can be upgraded either by re-installing the Alien, or by using af.

But it's unlikely that users know about this procedure (or ALIEN_INSTALL_TYPE), and many even won't be aware of the libcmark binary that was downloaded and installed behind their backs. If users are forced to install libcmark manually, they at least know about the dependency.

I certainly agree that this is a potential concern, however, I also believe that users that are processing random input off the internet without auditing the modules that they are using to process it, and their dependencies probably have a lot more to worry about that missing an update for libcmark.

For Alien::cmark, the version seems to be hardcoded, maybe because there's simply no way to discover the latest GitHub release. Anyway, the libcmark API isn't stable yet and automatically downloading the latest release isn't a good idea as it could suddenly break things. But there should be an easy way for users to install or upgrade to a specific libcmark version on all platforms.

@Grinnz can probably explain better than myself, but I think maybe you answered your own question? If the API isn't stable downloading the latest stable version doesn't mean anything. If/when it does stabilize it might make sense to use the latest stable (or latest stable within a range perhaps?). Any of these are pretty easy to do from the alienfile.

Currently the "easy" way to upgrade a specific libcmark is to download it manually and install into a directory and use INC and LIBS. With the Alien you can do the same thing though the incantation is with PKG_CONFIG_PATH instead. With appropriate documentation I don't see that this is any harder at least. For my uses PKG_CONFIG_PATH is actually easier as if you want to always use an arbitrary build over the system one, you can just set PKG_CONFIG_PATH in your startup files.

Requiring explicit user request does sort of defeat the purpose of making it easy to install out of the box.
Yes, but for now, I'd prefer a bit of inconvenience over the issues mentioned above.

No problem, this is your module and it is up to you. I am sure that @Grinnz and I can help re-work this PR to make this opt-in as you prefer.

We have modified a number of CPAN modules to use Aliens and haven't really had any adverse problems as a result.

@Grinnz
Copy link
Author

Grinnz commented Apr 8, 2019

Regarding Alien::cmark's hardcoded version - It could be updated to retrieve the latest version from github but as you noted this may not be desirable. It may also be possible to retrieve the latest version at a maximum (not inclusive) of 1.0.0, until Alien::cmark itself is updated to the 1.0.0 branch for example. I am willing to set that up however you wish, there are no other consumers of the Alien at the moment. You can also require that Alien::cmark provide a specific version range like the example atleast_version in the PR - this won't change what it will download and build, but will bail out if the share or system version it's providing isn't a satisfactory version.

@plicease
Copy link

For module authors such as yourself would prefer an explicit opt-in from users before falling back on an Alien, I think the best mechanism would be to fallback on an Alien if the ALIEN_INSTALL_TYPE environment variable. The values for this variable have been documented for quite some time as part of the AB manual here:

https://metacpan.org/pod/Alien::Build#ALIEN_INSTALL_TYPE

As mentioned above values of system and share are typically used to override probe logic in the Alien itself, and default was also added to allow the variable to be set, but to retain the Alien's own probe logic. It is reasonable therefore that assume that if this environment variable is set that the user is aware of the Alien system and has consented to its use. For these reasons I think that setting this environment variable is the best interface to opt-in for alien consumers that want an explicit opt-in. The problem with an interactive prompt is that popular installers such as cpanm turn interactive controls off, although it might be reasonable to have an interactive prompt in addition to the environment variable interface.

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.

None yet

4 participants