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

Add supports_param function #310

Closed
wants to merge 1 commit into from

Conversation

cpansprout
Copy link

It is getting quite common these days to fill Makefiles.PL with version checks, because ExtUtils::MakeMaker provides no other way to probe its functionality. This is something I will not do, because I think it’s crazy. My approaches tend to be more draconian: https://metacpan.org/source/SPROUT/Acme-Eatemup-0.02/Makefile.PL.

Of course, ideally, instead of working around the lack of the feature, I should be a good citizen and propose a new feature to fill the gap. So here is a patch that implements supports_param, which can be used by future Makefiles.PL (when EUMM 7.32 is standard) to probe for features, instead of looking at the version number.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
So that, at some hoped-for time in the future, we can stop filling
Makefiles.PL with silly version checks!
@mohawk2
Copy link
Member

mohawk2 commented Dec 8, 2017

This API would be even neater:

WriteMakefile(
  param_if_supported(MIN_PERL_VERSION => ...),
  ...
)

Returning an empty list if unsupported. This achieves some DRY.

@Leont
Copy link
Member

Leont commented Dec 8, 2017

Given that one would need to configure require 7.32 to use this, I don't think this is particularly useful. It's literally useless for any past feature, and for any future feature (I don't think we've added one in about 2 years, they're not particularly common anymore) one can easily use configure-requires as well.

@mohawk2
Copy link
Member

mohawk2 commented Dec 8, 2017

I agree with @Leont.

@kentfredric
Copy link
Contributor

It would probably be a better use of time to create a text file somewhere that people can consult, similar to CHANGES, but from the aspect of "I need syntax Y, what version does that need".

Then that text file can be referred to when generating compatibility code in Makefile.PL, and truely support a broad range of versions. How one wishes to make that happen in Makefile.PL is up to them, but adding a feature to detect the presence of features added before it seems .... well.... putting the cart before the horse, so to speak.

( But for instance, somebody who was keen could make a 3rd party CPAN module that was bundleable in inc/ which could then be loaded on even old EUMMs and then do something with magic syntax to do feature detection, and this wouldn't suffer the same issue of needing a newer feature of EUMM )

@Grinnz
Copy link
Contributor

Grinnz commented Dec 8, 2017

That "text file" can mostly be gleaned from the current pod of EUMM. A good next step would be a CPAN module that has a simple API to determine feature support, either what features are available at a certain version or what version is required for a certain feature.

@Grinnz
Copy link
Contributor

Grinnz commented Dec 8, 2017

And I am volunteering to write such a module, but I'd leave it up to others to determine how they want to use it for inc/ bundling or whatever other magic.

@mohawk2
Copy link
Member

mohawk2 commented Dec 8, 2017

Here is a start for the data of what is supported and when, with a suggestion of a format (CSV of key, supportedfrom, actionifnot, actionarg):

META_MERGE,6.45_01,delete
CONFIGURE_REQUIRES,6.51_03,move_to,PREREQ_PM
BUILD_REQUIRES,6.55_01,move_to,PREREQ_PM
TEST_REQUIRES,6.63_03,move_to,PREREQ_PM

And I humbly submit that the move_to function on this hypothetical module should be based on this: https://github.com/ilmari/dbicx-autodoc/blob/7e09bbbaafbe37a3096924a675445d277a482596/Makefile.PL#L50-L65

@Leont
Copy link
Member

Leont commented Dec 8, 2017

And I am volunteering to write such a module, but I'd leave it up to others to determine how they want to use it for inc/ bundling or whatever other magic.

There are already boilerplate Makefile.PLs; taking one and making it a module would be the approach of least resistance.

@ap
Copy link

ap commented Dec 8, 2017

And I am volunteering to write such a module, but I'd leave it up to others to determine how they want to use it for inc/ bundling or whatever other magic.

Problem is you’d need to use it from within Makefile.PL, so you can’t simply depend on it; you need to pull it in via configure_requires. That’s much better than configure-requiring a whole new version of EU::MM… but it’s still less than ideal.

My own view of it is that EU::MM is in a position akin to autoconf – you should ship something full of autogenerated boilerplate that optimises for users’ ability to install the package without dependencies on anything on the user’s machine. Trying to fashion DRY APIs for use inside Makefile.PL itself optimises for author comfort in hand-maintaining that file instead.

If you want author comfort, write a tool to generate the boilerplate for you at authoring time. (That was the first reason I jumped on the Dist::Zilla bandwagon early on, and it’s the last reason that will be keeping me on it in the end, even as I’m recognising Dist::Zilla as mostly a mistake and steadily reducing my reliance on it. (The choice of problems it tries to solve is sound, mind you; just the particular solution and implementation is deeply problematic.))

This knowledge about what feature is in what version of EU::MM should be machine-encoded into an authoring tool – or just a Makefile.PL template – used at authoring time by the author. Not a module that runs at installation time on the user’s machine.

@Grinnz
Copy link
Contributor

Grinnz commented Dec 8, 2017

@ap - I agree with all of that, the module I am considering is simply a tool for tools such as you propose, which would be the 'further magic'. It can be bundled in inc and used that way, or it can be used to generate appropriate boilerplate on the author side and never be used in the dist. Or it can be configure_required if one so chooses. I would probably be inclined to include some form of that boilerplate generation (but maybe not at first).

@ap
Copy link

ap commented Dec 8, 2017

This is something I will not do, because I think it’s crazy

Why?

I understand feature probing in the context of e.g. browsers where you have multiple competing implementations that support different selections of features at different versions. I also understand it in the context of software with optional features, e.g. perl itself which can be compiled with threads enabled or disabled etc.… or, say, vim, which can be compiled with or without several language interpreters embedded etc.

But in the case of a single implementation that installs with a fixed set of features, where the presence/absence of a feature is fully described by which version you’re running…? What is the rationale for feature detection over version checks in that case?

To me, it’s the magic in your Makefile.PL that’s crazy, given that it does not appear to be warranted.

@cpansprout
Copy link
Author

Given that one would need to configure require 7.32 to use this, I don't think this is particularly useful.

It is fairly trivial to monkey-patch such a feature into EUMM, but very fragile unless the list of versions that need monkey-patching is finite. Currently, the list is open-ended. My patch would solve that problem (monkey-patching being required only up to 7.30).

It's literally useless for any past feature, and for any future feature (I don't think we've added one in about 2 years, they're not particularly common anymore) one can easily use configure-requires as well.

configure_requires does not work in 5.10, which all my modules support, and I do not believe in forcing users to install a version that is not actually needed for installation.

@cpansprout
Copy link
Author

It would probably be a better use of time to create a text file somewhere that people can consult, similar to CHANGES, but from the aspect of "I need syntax Y, what version does that need".

( But for instance, somebody who was keen could make a 3rd party CPAN module that was bundleable in inc/ which could then be loaded on even old EUMMs and then do something with magic syntax to do feature detection, and this wouldn't suffer the same issue of needing a newer feature of EUMM )

But such an approach is inherently fragile, because in my experience things tend to get out of sync, unless the synchronization is automated. If new versions of EUMM would provide an API to provide a list of supported parameters (a more generally API than my proposed patch), then said automation could use it.

@cpansprout
Copy link
Author

But in the case of a single implementation with a fixed set of features, where the presence/absence of a feature is fully described by which version you’re running…? What is the rationale for feature detection over version checks in that case?

Lists of version checks result in code that is hard to read and maintain. If all optional features are just listed in a hash (or hashes) that the code processes, maintenance and code clarity both improve.

@cpansprout
Copy link
Author

cpansprout commented Dec 9, 2017

To me, it’s the magic in your Makefile.PL that’s crazy, given that it does not appear to be warranted.

Yes, but writing that saved me having to go hunting for lists of versions and risking copying and pasting the wrong version into the wrong spot—something I have a tendency to do.

@Leont
Copy link
Member

Leont commented Dec 9, 2017

If new versions of EUMM would provide an API to provide a list of supported parameters (a more generally API than my proposed patch), then said automation could use it.

That wouldn't be enough to be generally useful. One would also need to know what to do when that feature is not supported. You appear to be assuming unsupported features can always be safely ignored, this is not the case.

@cpansprout
Copy link
Author

cpansprout commented Dec 9, 2017

That wouldn't be enough to be generally useful. One would also need to know what to do when that feature is not supported.

Yes, but you need to know that the feature is not supported before you can even decide what to do instead.

You appear to be assuming unsupported features can always be safely ignored, this is not the case.

I am not assuming that.

Is Module::CoreList not useful? I’m sure it is used in many ways that were not anticipated when it was first written.

All I am asking is that EUMM make available the list of known parameters that it already has. Maybe my proposed patch is not the best solution, since it is not versatile. But EUMM could still help when it comes to the alternatives proposed; e.g., by providing a function that lists all known parameters. Then callers can do what they want with it, from looking for specific items in the list, to generating modules that provide that list in different formats.

@karenetheridge
Copy link
Member

All I am asking is that EUMM make available the list of known parameters that it already has and knows.

I would posit that the best first place to do that is in documentation (including what the Makefile.PL should do if the feature is not available (e.g. when there is no TEST_REQUIRES, fold those prereqs into BUILD_REQUIRES, etc..). Even that alone has significant value to everyone out there today writing their own fallback routines. (Maybe the next step after that is presenting the data in a structured fashion that code can access; that will let authors experiment with various ways of using it.)

@cpansprout
Copy link
Author

I would posit that the best first place to do that is in documentation (including what the Makefile.PL should do if the feature is not available (e.g. when there is no TEST_REQUIRES, fold those prereqs into BUILD_REQUIRES, etc..). Even that alone has significant value to everyone out there today writing their own fallback routines.

That might be nice.

(Maybe the next step after that is presenting the data in a structured fashion that code can access; that will let authors experiment with various ways of using it.)

This does not have to be a ‘next step’. It can be done first, or at the same time.

@cpansprout
Copy link
Author

What’s interesting is that EUMM actually used to have such a feature (until 6.42), but it was undocumented.

Examples of its use:
https://metacpan.org/source/MIKER/Mail-SpamCannibal-1.08/fixup/perl2html.pl#L33
https://metacpan.org/source/MDOOTSON/Wx-0.9932/build/Wx/build/MakeMaker/Any_OS.pm#L17

It’s a pity it stopped working.

@mohawk2
Copy link
Member

mohawk2 commented Dec 10, 2017

I'm a big fan of introspection and would support an EUMM method that provided such.

@Leont
Copy link
Member

Leont commented Dec 11, 2017

What’s interesting is that EUMM actually used to have such a feature (until 6.42), but it was undocumented.

I agree that regression is unfortunate. It appears it wasn't noticed back then (or at least I'm unable to find a bug report about it). Overly aggressive modernization can easily cause this sort of issue.

@cpansprout
Copy link
Author

Ok, so what patch would be more likely to be accepted?
• One that restores %ExtUtils::MakeMaker::Recognized_Att_Keys and documents it
• One that provides an ExtUtils::MakeMaker::supported_params function returning a list

@Grinnz
Copy link
Contributor

Grinnz commented Dec 21, 2017

FWIW I have released https://metacpan.org/release/DBOOK/ExtUtils-MakeMaker-Attributes-0.001 which could potentially be used to help solve this problem. It doesn't do anything on its own but is intended to be a tool for building whichever type of implement one wishes to use. I intend to include a Makefile.PL boilerplate generator but haven't decided how to correctly merge arbitrary prereq lists without requiring that CPAN::Meta::Requirements is installed.

@ap
Copy link

ap commented Dec 22, 2017

Ok, so what patch would be more likely to be accepted?

I’m not in a position to accept or reject patches but I’d strongly argue in favour of restoring and documenting %Recognized_Att_Keys.

That would make code that works with the new MakeMaker release also automatically work with older releases – except for a defined stretch of releases in the middle. Adding support for those via
a monkeypatch in Makefile.PL instead of configure_requires becomes a fairly reasonable argument, and fairly likely to happen in practice.

Also, this route only requires a single form of monkeypatch. If you were to add a new function, then it would have to be monkeypatched into all old MakeMakers, and that opens up the question of whether to have two forms depending on whether the given MakeMaker has %Recognized_Att_Keys: either you have a universal form of monkeypatch, even for MakeMakers that already have code for what you need; or, you have two forms of monkeypatch. Both answers are kinda unsatisfying…

Overall, restoring %Recognized_Att_Keys is the simpler and more robust option from an entirety-of-CPAN perspective.

@Leont
Copy link
Member

Leont commented Dec 27, 2017

I don't see any argument against restoring previously existing functionality, even if I'm not convinced this sort of functionality is a good solution for this sort of problem.

@cpansprout
Copy link
Author

I have just opened pull request #312, which reinstates %Recognized_Att_Keys.

@cpansprout cpansprout closed this Dec 31, 2017
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

7 participants