| Message ID | 1434711274-49716-2-git-send-email-benoit@wsystem.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Benoît Thébaudeau, On Fri, 19 Jun 2015 12:54:34 +0200, Benoît Thébaudeau wrote: > Now that building the openssl binary without MMU is supported, the only > reason left for not building apps if the openssl binary is disabled is > to save build time. Moreover, the commit > 720893b62510438237b9923d744dd079ddb4f67d "openssl: disable apps for > NOMMU", which added this behavior, had a side effect: the scripts from > apps (CA.pl, CA.sh and tsget) and the default configuration file > (openssl.cnf) were no longer installed, which is not advertized by the > BR2_PACKAGE_OPENSSL_BIN option. CA.pl and CA.sh use the openssl binary, > so not installing them without the latter would make sense. But tsget But that's exactly what you're doing here: CA.pl and CA.sh are now installed, even if the openssl binary is not. Also, all the c_* programs in /etc/ssl/misc/ are installed, they call the openssl tool, but the openssl tool is not installed. tsget and CA.pl are perl scripts, so they cannot work if you don't have a perl interpreter on the target anyway. So, maybe we need something like: ifeq ($(BR2_PACKAGE_PERL),) define OPENSSL_REMOVE_PERL_SCRIPTS $(RM) -f $(TARGET_DIR)/etc/ssl/misc/{CA.pl,tsget} endef OPENSSL_POST_INSTALL_TARGET_HOOKS += OPENSSL_REMOVE_PERL_SCRIPTS endif ifeq ($(BR2_PACKAGE_OPENSSL_BIN),) define OPENSSL_REMOVE_BIN $(RM) -f $(TARGET_DIR)/usr/bin/openssl $(RM) -f $(TARGET_DIR)/etc/ssl/misc/{c_*,CA.pl,CA.sh} endef OPENSSL_POST_INSTALL_TARGET_HOOKS += OPENSSL_REMOVE_BIN endif No? Thomas
Dear Thomas Petazzoni, On 1 Jul 2015 11:14, Thomas Petazzoni wrote: > On Fri, 19 Jun 2015 12:54:34 +0200, Benoît Thébaudeau wrote: >> Now that building the openssl binary without MMU is supported, the only >> reason left for not building apps if the openssl binary is disabled is >> to save build time. Moreover, the commit >> 720893b62510438237b9923d744dd079ddb4f67d "openssl: disable apps for >> NOMMU", which added this behavior, had a side effect: the scripts from >> apps (CA.pl, CA.sh and tsget) and the default configuration file >> (openssl.cnf) were no longer installed, which is not advertized by the >> BR2_PACKAGE_OPENSSL_BIN option. CA.pl and CA.sh use the openssl binary, >> so not installing them without the latter would make sense. But tsget > > But that's exactly what you're doing here: CA.pl and CA.sh are now > installed, even if the openssl binary is not. Also, all the c_* > programs in /etc/ssl/misc/ are installed, they call the openssl tool, > but the openssl tool is not installed. > > tsget and CA.pl are perl scripts, so they cannot work if you don't have > a perl interpreter on the target anyway. > > So, maybe we need something like: > > ifeq ($(BR2_PACKAGE_PERL),) > define OPENSSL_REMOVE_PERL_SCRIPTS > $(RM) -f $(TARGET_DIR)/etc/ssl/misc/{CA.pl,tsget} > endef > OPENSSL_POST_INSTALL_TARGET_HOOKS += OPENSSL_REMOVE_PERL_SCRIPTS > endif > > ifeq ($(BR2_PACKAGE_OPENSSL_BIN),) > define OPENSSL_REMOVE_BIN > $(RM) -f $(TARGET_DIR)/usr/bin/openssl > $(RM) -f $(TARGET_DIR)/etc/ssl/misc/{c_*,CA.pl,CA.sh} > endef > OPENSSL_POST_INSTALL_TARGET_HOOKS += OPENSSL_REMOVE_BIN > endif > > No? Yes. My intent was to keep the rules minimal, but I agree that it is better as you suggest. I will send a v2. I have a question about the management of the scripts depending on Perl, though. Doing as you suggest hides this behavior in the .mk, so the users won't know that they have a choice just by looking at the configuration. Do you think that it does not really matter, or that a comment or a depends on / select should be added to the Config.in? Best regards, Benoît
Dear Benoît Thébaudeau, On Thu, 2 Jul 2015 11:58:01 +0200, Benoît Thébaudeau wrote: > Yes. My intent was to keep the rules minimal, but I agree that it is better as > you suggest. I will send a v2. Great, thanks. > I have a question about the management of the scripts depending on Perl, though. > Doing as you suggest hides this behavior in the .mk, so the users won't know > that they have a choice just by looking at the configuration. Do you think that > it does not really matter, or that a comment or a depends on / select should be > added to the Config.in? We generally try to not clutter menuconfig with too many comments/options: we can't make visible in menuconfig every little possible dependency. That's why we use a lot of "automatic optional dependencies": a package automatically uses another package if it is available, without having this dependency visible from a menuconfig/Config.in point of view. I think it's a bit the same here. If you know you need that specific Perl script from OpenSSL, then we assume that you are smart enough to realize that you might need to enable a Perl interpreter for your target. Yes: we do believe our users are smart! :-) Thanks, Thomas
On 07/02/15 12:04, Thomas Petazzoni wrote: > Dear Benoît Thébaudeau, > > On Thu, 2 Jul 2015 11:58:01 +0200, Benoît Thébaudeau wrote: > >> Yes. My intent was to keep the rules minimal, but I agree that it is better as >> you suggest. I will send a v2. > > Great, thanks. > >> I have a question about the management of the scripts depending on Perl, though. >> Doing as you suggest hides this behavior in the .mk, so the users won't know >> that they have a choice just by looking at the configuration. Do you think that >> it does not really matter, or that a comment or a depends on / select should be >> added to the Config.in? > > We generally try to not clutter menuconfig with too many > comments/options: we can't make visible in menuconfig every little > possible dependency. That's why we use a lot of "automatic optional > dependencies": a package automatically uses another package if it is > available, without having this dependency visible from a > menuconfig/Config.in point of view. We (or at least, I) do like to have such a thing mentioned in the help text, however. The help text does not clutter the menus so there is no reason to be terse there. Regards, Arnout > > I think it's a bit the same here. If you know you need that specific > Perl script from OpenSSL, then we assume that you are smart enough to > realize that you might need to enable a Perl interpreter for your > target. > > Yes: we do believe our users are smart! :-) > > Thanks, > > Thomas >
Arnout, On Thu, 2 Jul 2015 22:54:56 +0200, Arnout Vandecappelle wrote: > > We generally try to not clutter menuconfig with too many > > comments/options: we can't make visible in menuconfig every little > > possible dependency. That's why we use a lot of "automatic optional > > dependencies": a package automatically uses another package if it is > > available, without having this dependency visible from a > > menuconfig/Config.in point of view. > > We (or at least, I) do like to have such a thing mentioned in the help text, > however. The help text does not clutter the menus so there is no reason to be > terse there. I am perfectly fine with having more details in the Config.in help text. However, do we really want to explicitly list all automatic optional dependencies in the Config.in help text? I believe it would definitely be hard to maintain. So the Config.in help text should probably be mostly used for things that aren't clearly visible by reading the .mk file. And automatic optional dependencies are usually pretty explicit by looking at the .mk file. Best regards, Thomas
On 07/02/15 23:22, Thomas Petazzoni wrote: > Arnout, > > On Thu, 2 Jul 2015 22:54:56 +0200, Arnout Vandecappelle wrote: > >>> We generally try to not clutter menuconfig with too many >>> comments/options: we can't make visible in menuconfig every little >>> possible dependency. That's why we use a lot of "automatic optional >>> dependencies": a package automatically uses another package if it is >>> available, without having this dependency visible from a >>> menuconfig/Config.in point of view. >> >> We (or at least, I) do like to have such a thing mentioned in the help text, >> however. The help text does not clutter the menus so there is no reason to be >> terse there. > > I am perfectly fine with having more details in the Config.in help text. > > However, do we really want to explicitly list all automatic optional > dependencies in the Config.in help text? I believe it would definitely > be hard to maintain. Why is it hard to maintain? When you add an automatic dependency to the .mk file, add it to the Config.in as well. No biggie. That said, I didn't mean that it should necessarily be added for everything. But certainly for things that are not immediately obvious. E.g. python support obviously requires python, openssl support is also pretty obvious. Although even there I wouldn't mind mentioning it in the help text. In this case, on the other hand, it's a tool called 'tsget' that needs perl - not so obvious. > So the Config.in help text should probably be > mostly used for things that aren't clearly visible by reading the .mk > file. And automatic optional dependencies are usually pretty explicit > by looking at the .mk file. Ugh, I wouldn't like it if we expect from our users that they not only read all the .mk files of the packages they want to use, but also understand what it all means. Regards, Arnout > > Best regards, > > Thomas >
Arnout, On Thu, 2 Jul 2015 23:38:34 +0200, Arnout Vandecappelle wrote: > > However, do we really want to explicitly list all automatic optional > > dependencies in the Config.in help text? I believe it would definitely > > be hard to maintain. > > Why is it hard to maintain? When you add an automatic dependency to the .mk > file, add it to the Config.in as well. No biggie. Well, just another thing to keep in sync. If we were to do that, then we could just as well not do any automatic optional dependency, and always have a Config.in sub-option to enable the dependency. > That said, I didn't mean that it should necessarily be added for everything. > But certainly for things that are not immediately obvious. E.g. python support > obviously requires python, openssl support is also pretty obvious. Although even > there I wouldn't mind mentioning it in the help text. > > In this case, on the other hand, it's a tool called 'tsget' that needs perl - > not so obvious. True. > > So the Config.in help text should probably be > > mostly used for things that aren't clearly visible by reading the .mk > > file. And automatic optional dependencies are usually pretty explicit > > by looking at the .mk file. > > Ugh, I wouldn't like it if we expect from our users that they not only read all > the .mk files of the packages they want to use, but also understand what it all > means. But in practice, it's very much the case today, due to these automatic optional dependency handling that we use. Best regards, Thomas
diff --git a/package/openssl/openssl.mk b/package/openssl/openssl.mk index 34a9830..d450f1e 100644 --- a/package/openssl/openssl.mk +++ b/package/openssl/openssl.mk @@ -14,14 +14,6 @@ HOST_OPENSSL_DEPENDENCIES = host-zlib OPENSSL_TARGET_ARCH = generic32 OPENSSL_CFLAGS = $(TARGET_CFLAGS) -ifeq ($(BR2_PACKAGE_OPENSSL_BIN),) -define OPENSSL_DISABLE_APPS - $(SED) '/^build_apps/! s/build_apps//' $(@D)/Makefile.org - $(SED) '/^DIRS=/ s/apps//' $(@D)/Makefile.org -endef -OPENSSL_PRE_CONFIGURE_HOOKS += OPENSSL_DISABLE_APPS -endif - ifeq ($(BR2_USE_MMU),) OPENSSL_CFLAGS += -DHAVE_FORK=0 endif @@ -140,6 +132,13 @@ endef OPENSSL_POST_INSTALL_TARGET_HOOKS += OPENSSL_INSTALL_FIXUPS_SHARED endif +ifneq ($(BR2_PACKAGE_OPENSSL_BIN),y) +define OPENSSL_REMOVE_OPENSSL_BIN + rm -f $(TARGET_DIR)/usr/bin/openssl +endef +OPENSSL_POST_INSTALL_TARGET_HOOKS += OPENSSL_REMOVE_OPENSSL_BIN +endif + ifneq ($(BR2_PACKAGE_OPENSSL_ENGINES),y) define OPENSSL_REMOVE_OPENSSL_ENGINES rm -rf $(TARGET_DIR)/usr/lib/engines
Now that building the openssl binary without MMU is supported, the only reason left for not building apps if the openssl binary is disabled is to save build time. Moreover, the commit 720893b62510438237b9923d744dd079ddb4f67d "openssl: disable apps for NOMMU", which added this behavior, had a side effect: the scripts from apps (CA.pl, CA.sh and tsget) and the default configuration file (openssl.cnf) were no longer installed, which is not advertized by the BR2_PACKAGE_OPENSSL_BIN option. CA.pl and CA.sh use the openssl binary, so not installing them without the latter would make sense. But tsget does not use the openssl binary, and openssl.cnf can be used by libcrypto, so it is preferable to handle BR2_PACKAGE_OPENSSL_BIN like before the commit mentioned above, i.e. to always build and install apps and to just remove the openssl binary afterwards if needed, which the current commit does. Signed-off-by: Benoît Thébaudeau <benoit@wsystem.com> --- package/openssl/openssl.mk | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)