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

Feature proposal: "-X exclusions-file" option for mconfig #14

Merged
merged 9 commits into from Oct 24, 2017

Conversation

arc
Copy link
Contributor

@arc arc commented Oct 23, 2017

Packages can use this option to list symbols that shouldn't bring in the corresponding units. For example, Perl need not provide support for BSD index(3) as an alternative to C89 strchr(3), but "index" is the name of a Perl builtin, so that string in the source files is misunderstood by metaconfig as an attempt to use the BSD function.

With this change, Perl can deal with this situation by adding "index" (and "rindex") to an exclusion list.

Perl is already using a version of mconfig that includes this new feature:

Perl/metaconfig@c3a1ef2
Perl/perl5@92c878f

Please let me know what you think — thank you!

Packages can use this option to list symbols that shouldn't bring in the
corresponding units. For example, Perl need not provide support for BSD
index(3) as an alternative to C89 strchr(3), but "index" is the name of a
Perl builtin, so that string in the source files is misunderstood by
metaconfig as an attempt to use the BSD function.

With this change, Perl can deal with this situation by adding "index" (and
"rindex") to an exclusion list.
Copy link
Owner

@rmanfredi rmanfredi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you think we also need to add support for the same -X in metaxref? Otherwise it would produce misleading output, listing symbols that metaconfig will ignore...

Can you please amend the pull-request to that effect?

I also spotted a minor omission: the usage help for metaconfig is not listing "-X file" (use lower-case). And the current convention is to align all options:

Usage: metaconfig .... [-X file]
....
-X: read symbol exclusions from file

Consistency is important :-)

I'm also wondering whether "packinit" should not be extended to have a "exclusion_file" variable pointing to a relative path under the top. That way, metaconfig could auto-pick the file. I'm really wondering, this is not rhetorical :-).

Because, of course, if you include that in the makefile, you can always say "make Configure" and list the exclusion file on the command line... I don't like things that are too magical and hidden. Yet I want comfort and error-prone / repeatable outcomes...

Anyway, thanks for this useful addition!

This usage has produced a depreaction warning since Perl 5.000, and was
finally removed in Perl 5.22.
Otherwise, when mconfig is run with that option, the metaxref would
confusingly refer to excluded symbols.
@arc
Copy link
Contributor Author

arc commented Oct 24, 2017

Hi, Rafael. I've pushed some more commits making the changes you've asked for.

I do like your suggestion of having the file named in .package, so the last two commits in this PR support an $exclusions_file variable, and cause packinit to prompt for and set it.

Please let me know if you have any other concerns, or you'd like me to change anything else.

Copy link
Owner

@rmanfredi rmanfredi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see two minor points still :-)

In packinit.SH, you refer to "mconfig -X". The "mconfig" script is just an intermediate step, the real program that is installed is "metaconfig".

The usage for -X stipulates FILE and should be file, since the usage says [-X file]. Pesky review, I'm aware. :-D

Good work. Do you have time to fix these now, or do you prefer me to integrate now and let you submit another pull-request later?

@arc
Copy link
Contributor Author

arc commented Oct 24, 2017

Nice catches :-) I've just pushed two more commits with tweaks for those — again, please let me know if you see anything else you'd like to be changed.

@rmanfredi rmanfredi merged commit f8e6e03 into rmanfredi:master Oct 24, 2017
arc added a commit to Perl/metaconfig that referenced this pull request Apr 19, 2018
In particular, this allows the -X option to be named in the .package file
(which would have saved some confusion for Tux).

This also reduces the amount of divergence from upstream dist, which goes
a small way to addressing #43.
arc added a commit to Perl/metaconfig that referenced this pull request Apr 19, 2018
This branch pulls in some of the later changes from rmanfredi/dist#14.
Raphaël suggested that the `.package` file be permitted to name the file
used for the `-X` option, and that `packinit` prompt for it. Those changes
have now been applied to our local copy of the (generated) `bin/mconfig`
file, and I've regenerated our `.package` file with the relevant setting.
Finally, our README no longer recommends the option, as it's no longer
needed.
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

2 participants