#50200 Ticket 50199 - disable perl by default
Closed by spichugi. Opened by firstyear.
firstyear/389-ds-base 50199-disable-perl  into  master

Download 50200.patch

Bug Description: Our python lib389 tools have become much
more mature. We should disable perl by default as it's really
not maintained, and deprecated, so we should stop emitting it
by default. It can still be enabled with --enable-perl to
./configure, but we just discourage it.

Fix Description: Turn yes to no.

https://pagure.io/389-ds-base/issue/50199

Author: William Brown william@blackhats.net.au

Review by: ???

Does this change remove perl deliverables from the RPM (db2ldif.pl, db2back.pl...) ?
It likely exist tons of specific admin scripts relying on those tools, IMHO we need to continue delivering them (and support them). Couldn't we change them so that they just call the new CLI and add a warning message that the script will be deprecated.

With 1.4, the Perl tools are essentially deprecated (they are in the -legacy-tools subpackage), with, let's say, removal in 1.5. It makes sense then to disable them by default. This change doesn't change the upstream SPEC file where this is explicitly set to yes, so no change when building RPMs. Sure, during lifetime of 1.4 we should ensure they still work but it seems to me that having them enabled by default in SPEC file should be sufficient. ACK from me, but let's wait for Mark's [@mreynolds], at least, as well.

@firstyear Could you also change condition in spec file? Otherwise build might fail.

https://pagure.io/389-ds-base/blob/master/f/rpm/389-ds-base.spec.in#_358

-%if !%{use_perl}
-PERL_FLAGS="--disable-perl"
+%if %{use_perl}
+PERL_FLAGS="--enable-perl"
%endif

^^^
Not tested :-)

We still need to support/ship the legacy tools until 1.4.2. They were only deprecated in 1.4.1 (Fedora 30). So if this impacts the legacy tools then we need to wait to commit this...

@mreynolds It doesn't impact the legacy tool at all. It leaves all the code there just fine. Because they are deprecated, by making them disabled people have to make an intentional and knowing action to enable in their packaging process. This way it highlights to people, yes, this change is coming.

It changes absolutely nothing for fedora/rhel, because all you do is add "--enable-perl" in your spec file, and be done with it.

So disabling them by default is a good change to make so that it's not so much of a surprise to future releases when they really really do get taken away...

@lslebodn Yes, but that spec is fedora specific and may not always match up with fedora/rhel

@lslebodn Yes, but that spec is fedora specific and may not always match up with fedora/rhel

I would call it upstream spec file which works on fedora. And because it is in upstream it would be great to have it working. And after this change it will not work.

BTW such spec might be an inspiration for fedora/rhel. It needn't be exact the same.

Feel free to add another one which will work on openSUSE (I am not sure how fedora and openSUSE packaging guidelines are compatible) :-)

I won't add a suse spec, because it turns out the OBS system is so easy to use, we can just use the suse spec :) Anyway, I'll put in the spec file fix today.

rebased onto dc741db152891488b8900f139aeacff05853ba90

@mreynolds Did you mind checking this?

I have not tested this because I am going on PTO, but the changes seem okay. (note to self) I will need to update the upstream/downstream specfiles

rebased onto 8e2da5db1b97f304990561032bb9e7bf44ca211d

Pull-Request has been merged by firstyear

Was this intentional? Just found it overrides the PERL_FLAGS two lines above. :( Shouldn't we better change the default e.g. in rpm.mk file?

Yes it was intentional : people asked me to re-enable it in the rpm spec for QE (I think), so I did just that ...

Ok. Then, again, why this way? Why not in the rpm.mk, or in the if !{use_perl} condition just a few lines above? Maybe call me blind but single source of truth is gone which helped me to at least an hour of debugging...

Because that's what RH QE asked me for, and they are the primary consumer of that RPM, and they still require perl because reasons?

If there is a simple fix to make this better, please add it, but I certainly don't use the rpm.mk file at all because it's not SUSE compatible and I don't want to make it SUSE compatible (I think we should remove it actually ....)

Because that's what RH QE asked me for, and they are the primary consumer of that RPM, and they still require perl because reasons?

If you are referring to https://pagure.io/389-ds-base/pull-request/50200#comment-74390 then you're not correct.
@lslebodn is not RH QE.
QE are not the only one using rpm.mk.
Perl tools are still required because of many reasons, including maintenance and support of legacy systems. I as QE would like to run tests on RHEL7 during it's support life cycle.

If there is a simple fix to make this better, please add it, but I certainly don't use the rpm.mk file at all because it's not SUSE compatible and I don't want to make it SUSE compatible (I think we should remove it actually ....)

You were provided with the fix in https://pagure.io/389-ds-base/pull-request/50200#comment-74390 but ignored it and did a different change instead that introduced an issue.

And if you're not using or understand something, it doesn't mean it should be removed.

Because that's what RH QE asked me for, and they are the primary consumer of that RPM, and they still require perl because reasons?

If you are referring to https://pagure.io/389-ds-base/pull-request/50200#comment-74390 then you're not correct.
@lslebodn is not RH QE.

I know he's not, apparently my memory is failing.

QE are not the only one using rpm.mk.
Perl tools are still required because of many reasons, including maintenance and support of legacy systems. I as QE would like to run tests on RHEL7 during it's support life cycle.

If there is a simple fix to make this better, please add it, but I certainly don't use the rpm.mk file at all because it's not SUSE compatible and I don't want to make it SUSE compatible (I think we should remove it actually ....)

You were provided with the fix in https://pagure.io/389-ds-base/pull-request/50200#comment-74390 but ignored it and did a different change instead that introduced an issue.

I didn't think I ignored it, I thought I applied it? Saying this, it also means that the "lack of fix" went through review, and people acked etc. Mistakes happen. We fix them, and we move on.

And if you're not using or understand something, it doesn't mean it should be removed.

I don't think it should be the job of upstream to support internal corporate use cases.

Anyway, I have no issues with the fixes that @mhonek is suggesting here, or the fix in that comment.

Thanks for the clarification. I'll post a PR fixing this soon.

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3259

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

Metadata