Ticket #797 (closed RFC: fixed)

Opened 5 years ago

Last modified 5 years ago

Eliminate need to expand Makefile variables in parrot_config and other external programs

Reported by: jkeenan Owned by: jkeenan
Priority: normal Milestone:
Component: configure Version: 1.3.0
Severity: medium Keywords: configure makefile
Cc: jhorwitz, allison Language:
Patch status: applied Platform: all

Description

This ticket represents conversion of a thread on parrot-dev into a Trac ticket. I am presenting the original post as well as excerpts from the follow-up discussion.

Jeff Horwitz wrote on 5/17/09:"

in #parrotsketch last week i brought up the interesting output of parrot_config, and allison asked for an RFC. here it is.

most parrot_config values are literal strings, simple enough. for example:

ldflags => ' -L/usr/local/lib'
libdir => '/usr/local/lib'
libexecdir => '/usr/local/libexec'

others, however, contain Makefile variables that would need to be expanded before use. for example:

libparrot => '$(LIBPARROT_SHARED)'
libparrot_shared => 'libparrot$(SHARE_EXT).$(SOVERSION)'
libparrot_shared_alias => 'libparrot$(SHARE_EXT)'
libparrot_soname => '-Wl,-soname=libparrot$(SHARE_EXT).$(SOVERSION)'

some are simple, but others, such as libparrot, would require several iterations to expand. when i want to know the name of the shared libparrot, the last thing i expect to see is libparrot$(SHARE_EXT).$(SOVERSION). it makes me work that much harder for a simple bit of information.

it also forces the use of a Makefile. for most projects that won't be a problem, but what if i want these values during a configuration stage, pre-Makefile? that's how GNU configure scripts work, and embedded variables would break it horribly. outside of configure, i could write something to expand the variables, but why not do that for the programmer in the config itself? and what if i have my own $(SOVERSION)? i have to rename it now?

so, to the point. is there a reason we're using embedded Makefile variables in parrot_config values? my guess is that it's legacy, and if that's the case i strongly recommend expanding these variables. the only consequence i could think of is what if a value changes? the answer here is to reconfigure -- if one of these values changes, you should probably be reconfiguring anyway.

comments please!

-jeff

Jim Keenan wrote on 6/22/09:

A data point:

./parrot_config --dump | grep '$('
cat => '$(PERL) -MExtUtils::Command -e cat'
chmod => '$(PERL) -MExtUtils::Command -e ExtUtils::Command::chmod'
cp => '$(PERL) -MExtUtils::Command -e cp'
libparrot => '$(LIBPARROT_SHARED)'
libparrot_shared => 'libparrot$(SHARE_EXT).$(SOVERSION)'
libparrot_shared_alias => 'libparrot$(SHARE_EXT)'
libparrot_soname => '-Wl,-soname=libparrot$(SHARE_EXT).$(SOVERSION)'
mkpath => '$(PERL) -MExtUtils::Command -e mkpath'
mv => '$(PERL) -MExtUtils::Command -e mv'
rm_f => '$(PERL) -MExtUtils::Command -e rm_f'
rm_rf => '$(PERL) -MExtUtils::Command -e rm_rf'
touch => '$(PERL) -MExtUtils::Command -e touch'

Jeff Horwitz wrote on 6/23/09:

i decided to dig a little deeper for the greater good. i found that the Makefile-style variables are hidden in the OS hints files. it doesn't make much sense to expand them there, but we might want to consider doing it while generating parrot_config. i already wrote the expansion code for mod_parrot -- i'll see if i can shoehorn it in.

Allison Randal wrote on 6/24/09:

Yes, having parrot_config pre-process these substitutions is a good idea. (We could add a command-line flag to turn off preprocessing when the raw values are really needed.)

Andy Dougherty wrote on 6/25/09:

Before adding in yet another layer of variable expansion, it might be worthwhile to step back as you did and ask where these variables came from, and question why they exist at all.

Most of them (cat, chmod, cp, mkpath, mv, rm_f, rm_rf, and touch) are documented in config/init/defaults.pm as '#some utilities in Makefile'. I don't know whether or not they are a problem. They are documented as variables to be used in a Makefile, and they work quite well in that context. However, they are also sufficiently vanilla commands that expanding out the $(PERL) would probably make little or no difference. It also may not be at all relevant, since I'm unclear whether you are trying to use these utility commands outside of a Makefile. If you are, you may be better off just using the ExtUtils::Command versions directly.

The remaining ones (libparrot, libparrot_shared, libparrot_shared_alias, and libparrot_soname) look to me as if they can (and should) simply be expanded by Configure.pl in the first place.

Andy Dougherty further wrote on 6/25/09:

If you look for SHARE_EXT, you'll pick up lots of lines like this: (I've collapsed spaces for clarity)

    config/init/hints/openbsd.pm:
      libparrot_shared  => 'libparrot$(SHARE_EXT).$(SOVERSION)',

I am proposing replacing that by

      libparrot_shared  => 'libparrot' . 
	      $conf->data->get('share_ext')
      				. "." . 
	      $conf->data->get('VERSION')

(since soversion isn't currently defined by Configure.pl, but ends up being VERSION, i.e., something like 1.3.0.)

The issue is Config variables getting set to stuff like$(STUFF) that will have to be further expanded by the Makefile and also, independently, and redundantly, expanded by the hypothetical parrot_config creation utility. My idea was to not do that. Simply have perl write the expanded values into Generated.pm in the first place. Then nobody has to expand or convert anything.

Attachments

libparrot.interim.patch Download (2.4 KB) - added by jkeenan 5 years ago.
patch so far for Linux only

Change History

  Changed 5 years ago by jkeenan

  • keywords configure makefile added
  • platform set to all
  • component changed from none to configure

follow-up: ↓ 3   Changed 5 years ago by jkeenan

  • status changed from new to assigned
  • owner set to jkeenan

I was able to pluck some low-hanging fruit on this ticket but have reached an impasse.

Focusing strictly on Linux, I wrote this patch:

Index: config/init/hints/linux.pm
===================================================================
--- config/init/hints/linux.pm  (revision 39830)
+++ config/init/hints/linux.pm  (working copy)
@@ -13,6 +13,8 @@
     my $ccflags   = $conf->option_or_data('ccflags');
     my $cc        = $conf->option_or_data('cc');
     my $linkflags = $conf->option_or_data('linkflags');
+    my $share_ext = $conf->option_or_data('share_ext');
+    my $version   = $conf->option_or_data('VERSION');
     my $verbose;
 
     $verbose = $conf->options->get('verbose');
@@ -142,9 +144,9 @@
 
         has_dynamic_linking    => 1,
         parrot_is_shared       => 1,
-        libparrot_shared       => 'libparrot$(SHARE_EXT).$(SOVERSION)',
-        libparrot_shared_alias => 'libparrot$(SHARE_EXT)',
-        libparrot_soname       => '-Wl,-soname=libparrot$(SHARE_EXT).$(SOVERSION)',
+        libparrot_shared       => "libparrot$share_ext.$version",
+        libparrot_shared_alias => "libparrot$share_ext",
+        libparrot_soname       => "-Wl,-soname=libparrot$share_ext.$version",
     );
 
      if ( ( split( m/-/, $conf->data->get_p5('archname'), 2 ) )[0] eq 'ia64' ) {

In other words, I simply hard coded $(SHARE_EXT) and $(SOVERSION) and eliminated them as Make variables. After running make, I got this diff of the output of parrot_config --dump:

$ diff -w before.dump after.dump 
90c90
< configdate => 'Mon Jun 29 11:53:51 2009 GMT'
---
> configdate => 'Mon Jun 29 12:30:02 2009 GMT'
207,209c207,209
< libparrot_shared => 'libparrot$(SHARE_EXT).$(SOVERSION)'
< libparrot_shared_alias => 'libparrot$(SHARE_EXT)'
< libparrot_soname => '-Wl,-soname=libparrot$(SHARE_EXT).$(SOVERSION)'
---
> libparrot_shared => 'libparrot.so.1.3.0'
> libparrot_shared_alias => 'libparrot.so'
> libparrot_soname => '-Wl,-soname=libparrot.so.1.3.0'

So far, so good. But in the above parrot_config still holds:

libparrot => '$(LIBPARROT_SHARED)'

... which is set in config/inter/libparrot.pm:

    $conf->data->set(
        parrot_is_shared => $parrot_is_shared,

        libparrot => $parrot_is_shared
        ? '$(LIBPARROT_SHARED)'
        : '$(LIBPARROT_STATIC)',

... and that $(LIBPARROT_SHARED) is needed for make, as per this section in config/gen/makefiles/root.in:

# Libraries
LIBPARROT_STATIC    := @blib_dir@/@libparrot_static@
#IF(darwin):export DYLD_LIBRARY_PATH := @blib_dir@:$(DYLD_LIBRARY_PATH)
#IF(win32):LIBPARROT_SHARED  := @libparrot_shared@
#ELSE:LIBPARROT_SHARED  := @blib_dir@/@libparrot_shared@

# This line controls whether a static or shared library is built
LIBPARROT           := @libparrot@

The makefile is composed by config/gen/makefiles.pm, the second-to-last configuration step. This config step needs certain attributes in the Parrot::Configure object to have been written in Makefile variable form, because the values of those attributes are targets within make. When I tried to immediately expand the value of $(LIBPARROT_SHARED) in config/inter/libparrot.pm, make failed because their was no target named with the expanded value.

The problem we face now is the fact that files like myconfig, lib/Parrot/Config/Generated.pm and config_lib.pasm are written by the very last configuration step, config/gen/config_pm.pm. My hunch is that we ultimately do not want any Makefile variables written to these files; we want their fully expanded forms. Unfortunately, gen::config_pm works from four template files -- 3 of which are coded to expect values in the @foo@ format characteristic of Makefiles.

That suggests that config/gen/config_pm.pm is going to have to do more work than it does right now; that we may have to rethink the way it composes the files it does; and that we may have to rethink the extent to which we store data in the Parrot::Configure object in the form of Makefile variables.

Suggestions? Thank you very much.

kid51

in reply to: ↑ 2 ; follow-ups: ↓ 4 ↓ 5   Changed 5 years ago by jkeenan

Replying to jkeenan:

That suggests that config/gen/config_pm.pm is going to have to do more work than it does right now; that we may have to rethink the way it composes the files it does; and that we may have to rethink the extent to which we store data in the Parrot::Configure object in the form of Makefile variables.

More specifically, should it be our goal to eliminate all conditional determination of configuration values from the Makefile? Why, for instance, do we need code like this in config/gen/makefiles/root.in:

LIBPARROT_STATIC    := @blib_dir@/@libparrot_static@
#IF(darwin):export DYLD_LIBRARY_PATH := @blib_dir@:$(DYLD_LIBRARY_PATH)
#IF(win32):LIBPARROT_SHARED  := @libparrot_shared@
#ELSE:LIBPARROT_SHARED  := @blib_dir@/@libparrot_shared@

By the second-to-last configuration step, don't we already know what OS we're on?

Moreover, if it is make that has the last word on, e.g., what the value of libparrot is,

# This line controls whether a static or shared library is built
LIBPARROT           := @libparrot@

... then none of the generated files or compiled programs which depend on values stored in lib/Parrot/Config/Generated.pm or config_lib.pasm would ever contain or have available to it the final values -- as make determines those values after those two files have been created by Configure.pl?

kid51

in reply to: ↑ 3   Changed 5 years ago by jkeenan

Replying to jkeenan:

More specifically, should it be our goal to eliminate all conditional determination of configuration values from the Makefile?

Granted, this would be a lot of work to get right. There are 52 #IF statements in config/gen/makefiles/root.in. But there are 460 instance of Makefile variables in a typical lib/Parrot/Config/Generated.pm. And there are 9 such variables in a typical config_lib.pasm.

kid51

in reply to: ↑ 3   Changed 5 years ago by doughera

Replying to jkeenan:

More specifically, should it be our goal to eliminate all conditional determination of configuration values from the Makefile?

That's probably not worth the effort. Things like

#IF(darwin):export DYLD_LIBRARY_PATH := @blib_dir@:$(DYLD_LIBRARY_PATH)

are relatively simple, straightforward, and easy to understand.

Why, for instance, do we need code like this in config/gen/makefiles/root.in:

> LIBPARROT_STATIC    := @blib_dir@/@libparrot_static@
> #IF(win32):LIBPARROT_SHARED  := @libparrot_shared@
> #ELSE:LIBPARROT_SHARED  := @blib_dir@/@libparrot_shared@

I know of no compelling reason. The variables "libparrot_static" and "libparrot_shared" aren't really documented that well (i.e. are they supposed to include the path or not?) so you end up with the confusing situation here where LIBPARROT_STATIC includes the path, but the almost-identically-named libparrot_static does not. Either is reasonable.

By the second-to-last configuration step, don't we already know what OS we're on?

Yes, we certainly do.

Moreover, if it is make that has the last word on, e.g., what the value of libparrot is,

> # This line controls whether a static or shared library is built
> LIBPARROT           := @libparrot@

I don't see a problem with this. Perhaps if the variable were named libparrot_target_for_makefile_in_the_build_directory_but_useless_otherwise then it would be obvious that it's not intended to be used outside of the build directory main makefile. As it is, it has a name that makes it look like it might be something useful to know, and there's no documentation to tell you otherwise.

... then none of the generated files or compiled programs which depend on values stored in lib/Parrot/Config/Generated.pm or config_lib.pasm would ever contain or have available to it the final values -- as make determines those values after those two files have been created by Configure.pl?

But I don't see why any of those generated files need to care either. That variable is only intended for use as a Makefile target in the build directory. Outside of that context, it is useless and irrelevant. I don't see anything wrong with "fixing" it, but I don't see much of a point either. I suspect effort could be better spent documenting parrot_config variables so it's clearer which ones are worth worrying about in an installed parrot and which ones aren't.

  Changed 5 years ago by doughera

  • cc jhorwitz, allison added; jhorwitz allison doughera removed

Changed 5 years ago by jkeenan

patch so far for Linux only

in reply to: ↑ description ; follow-up: ↓ 8   Changed 5 years ago by jkeenan

jhorwitz, allison:

Is the attached interim patch (so far, Linux only) a step in the right direction?

kid51

in reply to: ↑ 7 ; follow-ups: ↓ 10 ↓ 11   Changed 5 years ago by jkeenan

Replying to jkeenan:

jhorwitz, allison: Is the attached interim patch (so far, Linux only) a step in the right direction?

Since there was no objection, I proceeded to do the same kind of revisions I did in the patch for config/init/hints/darwin.pm. I then examined lib/Parrot/Config/Generated.pm and config_lib.pasm and got satisfactory results. make then proceeded without incident.

Buoyed by those results, I then extended those revisions to all the OSes for which there exist config/init/hints/*.pm files and which held either SHARE_EXT or SOVERSION. I then applied the patch to trunk in r39871.

Granted, this means I'm applying a patch to hints for OSes to which I do not have access. However, we're at a point in the monthly cycle where we can get away with this because we will get build failures quickly (I hope!).

Comments? Thank you very much.

kid51

  Changed 5 years ago by jkeenan

  • patch set to applied

in reply to: ↑ 8   Changed 5 years ago by jkeenan

Replying to jkeenan:

Granted, this means I'm applying a patch to hints for OSes to which I do not have access. However, we're at a point in the monthly cycle where we can get away with this because we will get build failures quickly (I hope!).

Since application of the patch, (a) we've had Smolder reports submitted for this or later revisions for all relevant OSes except Dragonfly (for which we have not had reports in 4 months); and (b) fperrad has explicitly confirmed no-harm-done on Win32.

So far, so good.

in reply to: ↑ 8 ; follow-up: ↓ 12   Changed 5 years ago by jkeenan

Replying to jkeenan:

Heard no complaints ... but, then again, I got no feedback whatsoever!

Here is the current output of parrot-config --dump:

76:cat => '$(PERL) -MExtUtils::Command -e cat'
89:chmod => '$(PERL) -MExtUtils::Command -e ExtUtils::Command::chmod'
94:cp => '$(PERL) -MExtUtils::Command -e cp'
205:libparrot_for_makefile_only => '$(LIBPARROT_SHARED)'
230:mkpath => '$(PERL) -MExtUtils::Command -e mkpath'
232:mv => '$(PERL) -MExtUtils::Command -e mv'
267:rm_f => '$(PERL) -MExtUtils::Command -e rm_f'
268:rm_rf => '$(PERL) -MExtUtils::Command -e rm_rf'
283:touch => '$(PERL) -MExtUtils::Command -e touch'

The one variable that was part of jhorwitz's original complaint, $(LIBPARROT_SHARED), is now clearly marked as for use in Makefile only. That leaves just the instances of $(PERL). I'm not sure we can do better at this time.

So if no one complains within 24 hours, I will close this ticket.

Thank you very much.
kid51

in reply to: ↑ 11   Changed 5 years ago by jkeenan

  • status changed from assigned to closed
  • resolution set to fixed

Replying to jkeenan:

So if no one complains within 24 hours, I will close this ticket.

24 hours ... and no one complained. Closing ticket.

kid51

Note: See TracTickets for help on using tickets.