Getting rid of Makefile.shared #4840
Conversation
| return grep /gcc/, @lines; | ||
| } | ||
|
|
||
| my %shared_info; |
There was a problem hiding this comment.
combine the two lines? And with that, my Perl expertise is all used up :)
There was a problem hiding this comment.
(I think... since I have references to %shared_info from within the subs in this structure, I'm worried that combining will mean the symbol isn't lexigraphically defined when those subs are compiled.
There was a problem hiding this comment.
Ahyup, when I combined into one line, I got this:
Global symbol "%shared_info" requires explicit package name (did you forget to declare "my %shared_info"?) at (eval 11) line 32.
Global symbol "%shared_info" requires explicit package name (did you forget to declare "my %shared_info"?) at (eval 11) line 36.
Global symbol "%shared_info" requires explicit package name (did you forget to declare "my %shared_info"?) at (eval 11) line 38.
Global symbol "%shared_info" requires explicit package name (did you forget to declare "my %shared_info"?) at (eval 11) line 54.
Global symbol "%shared_info" requires explicit package name (did you forget to declare "my %shared_info"?) at (eval 11) line 61.
Global symbol "%shared_info" requires explicit package name (did you forget to declare "my %shared_info"?) at (eval 11) line 68.
Global symbol "%shared_info" requires explicit package name (did you forget to declare "my %shared_info"?) at (eval 11) line 75.
Global symbol "%shared_info" requires explicit package name (did you forget to declare "my %shared_info"?) at (eval 11) line 82.
Global symbol "%shared_info" requires explicit package name (did you forget to declare "my %shared_info"?) at (eval 11) line 89.
Global symbol "%shared_info" requires explicit package name (did you forget to declare "my %shared_info"?) at (eval 11) line 96.
So, errr, no.
Configurations/unix-Makefile.tmpl
Outdated
| $shared_imp .= ' '.$target{shared_impflag}.basename($target) | ||
| if defined $target{shared_impflag}; | ||
| my $shared_def = join("", map { ' '.$target{shared_defflag}.$_ } @defs); | ||
| my $recipe = <<"EOF"; | ||
| # With a build on a Windows POSIX layer (Cygwin or Mingw), we know for a fact |
There was a problem hiding this comment.
Since you're in the area, should "With a build on..." be changed to "When building on..." ? Side-note.
Configure
Outdated
| } | ||
| } | ||
| use Data::Dumper; | ||
| print STDERR Dumper({ %target }); |
|
Hmmm, I think I need to double check on Windows... Appveryor seems to hang forever during configuration... |
levitte
force-pushed
the
getting-rid-of-Makefile.shared
branch
from
c13cba8
to
e25f61a
Compare
|
Hah! Windows problem resolved, or so I hope. Please reconfirm (you can just look at the new commit if you prefer... it will be squashed in appropriately when I'll merge) |
|
reconfirm after the builds are done. |
| sub detect_gnu_cc { | ||
| my @lines = | ||
| `$config{cross_compile_prefix}$target{cc} -v 2>&1`; | ||
| return grep /gcc/, @lines; |
There was a problem hiding this comment.
As pointed out in #4755, this is not reliable way to detect gcc.
There was a problem hiding this comment.
Ah... ok, just gotta ensure %predefined is filled in early enough, then.
There was a problem hiding this comment.
... which isn't so easy, we have a bit of a catch 22 there, since $target{cc} is modified far later than these functions are called. Note that his is copied directly from Makefile.shared, and also, this is only called for svr5-shared, where I'm unsure there's been any report of an issue (not that we get much reports from that platform anyway). So I'd like to argue that we can leave this alone for this PR and solve in a more general in a separate effort (I know you're thinking of options). Sounds reasonable to you?
There was a problem hiding this comment.
Here is a wild idea... Why can't those flags that are defined here in shared-info.pl be just a template? I mean why can't a target be described as inherit_from => [ ..., "something-shared", ... ]? Why is there additional level of "indirection" with shared_target "pointing" to separate data structure? Wouldn't it be sufficient if shared_target was rather just a "reference" to a subroutine that accepts complete target description as argument? And not dedicated data structure and target that is?
There was a problem hiding this comment.
'cause inherit_from is processed before the final target structure is done, and because the subs that are values for attributes only receive the inherited value for that same attribute. In the other PR, where I implement later processing of those subs, it may be possible to get any target value from any sub, that will make things easier...
There was a problem hiding this comment.
Forgot to say that some of those shared-info.pl structures depend on $target{cc}, so you can see how that's a problem to handle with inherit_from
There was a problem hiding this comment.
And the question is does it have to be that way? One can also put it in following way. This is kind of dominated by idea that it has to mimic Makefile.shared, but does it actually have to?
Configurations/shared-info.pl
Outdated
| 'bsd-shared' => sub { | ||
| return $shared_info{'gnu-shared'} if detect_gnu_ld(); | ||
| return { | ||
| shared_ldflag => '-shared -nostlib', |
Configurations/shared-info.pl
Outdated
| }; | ||
| }, | ||
| 'solaris-shared' => sub { | ||
| return $shared_info{'gnu-shared'} if detect_gnu_ld(); |
There was a problem hiding this comment.
This can be removed, because there is no gnu ld on Solaris, and even gcc uses vendor linker.
There was a problem hiding this comment.
Ok... this came from early experimentation on Solaris around 2000, and I don't quite recall on what platform I saw the need for this. I guess I got paranoid and did this for all platforms where we used gcc at all.
Will fix.
| shared_ldflag => '-G', | ||
| shared_sonameflag => '-h ', | ||
| }; | ||
| }, |
There was a problem hiding this comment.
One can wonder what are the options of having this in .conf files. I mean this SCO stuff doesn't really belong in 10-main.conf. So if there are any nostalgic feelings it, SCO stuff should rather be moved to 80-sco.conf, and it such case it would be only appropriate if this is also moved. So that it can be later deleted in one step...
There was a problem hiding this comment.
I'm actually thinking of making config templates instead of this special case file, or where appropriate, just insert these values in already existing templates. We could also get rid of the shared_target attribute and add a enable => [ 'shared' ] instead (since we already have the processing of such attributes in place).
There was a problem hiding this comment.
Ah, sorry, this will actually be difficult to do. The reason is that the check of GNU ld in some of the shared targets would need to happen in inherit_from, which is computed early ('cause reasons), even in #4854. $target{cc} isn't defined then, getting us back in a catch 22 that is even harder to resolve than the one I mentioned above...
(I think it's still possible, but will be an uglier hack...)
Configurations/shared-info.pl
Outdated
| }; | ||
| }, | ||
| 'hpux-shared' => sub { | ||
| return $shared_info{'gnu-shared'} if detect_gnu_ld(); |
There was a problem hiding this comment.
I'm pretty sure there is no gnu ld on HP-UX either...
There was a problem hiding this comment.
"Pretty sure" means that I don't see one on 11 and have hard time imagining that there was one earlier.
There was a problem hiding this comment.
Ok, I'll take the chance and remove this then...
build.info
Outdated
| SHARED_SOURCE[libssl]=libssl.symvec | ||
| ENDIF | ||
|
|
||
| IF[{- $config{target} =~ /^(?:Cygwin|mingw|VC-)/ -}] |
There was a problem hiding this comment.
Wouldn't VC\- be more appropriate? I mean since dash has special meaning. Well, it does work now...
There was a problem hiding this comment.
The dash is only special within [], so no.
There was a problem hiding this comment.
and escaping - in [] is not done with \ (doing so would include literal \), but rather having it as last or first char inside [].
levitte
force-pushed
the
getting-rid-of-Makefile.shared
branch
from
8d9c71c
to
50f6db3
Compare
|
Travis has a failure in |
|
@dot-asm, you might be interested in looking at a branch where I have merged together this PR, #4854 and #4899, found here, to show how it all comes together. |
…luator It will return the last expression from the input file. We also use this in read_config, which slightly changes what's expected of Configurations/*.conf. They do not have to assign %targets specifically. On the other hand, the table of configs MUST be the last expression in each of those files.
levitte
force-pushed
the
getting-rid-of-Makefile.shared
branch
from
50f6db3
to
406e46b
Compare
|
If anything, this will make those who appreciate the silence of |
|
There's a reason why I approved this :) And reconfirm now. |
|
I hear ya... but I'm gonna wait for @dot-asm to say his piece as well... and I just realised that there are things I haven't fixed that I said I would, so ;-) |
|
Red cross from appveyor ought to be relevant. |
|
I get failures on MacOS X. Quite old though. Could you temporarily throw osx back into .travis.yml? |
|
As you can see from the fixup / squash commits I'm throwing in, I'm currently watching all CIs quite closely ;-) Sure, I'll trow in osx as well (should we make that a permanent thing?) |
It was taken away because it was common queue, and as results of that request turn-around was effectively bound to backlog on osx, which was huge [during working days]. And still is according to https://www.traviscistatus.com/#week, so it's useless for general use... |
|
Ah, yeah, that doesn't look too good |
|
Seeing some preliminary osx results... they get stuck at |
|
I get errors linking shared library! I suppose I have to get my hands on newer mac... |
|
BTW, the error I get is |
This makes it possible to add build.info statements for using resource files as well as linker scripts (.def for Windows, .map for Unix, and .opt for VMS) is if they were source files. This requires changes in the build file templates.
Because this also includes handling all sorts of non-object files when linking a program, shared library or DSO, this also includes allowing general recognition of files such as .res files (compiled from .rc files), or .def / .map / .opt files (for export and possibly versioning of public symbols only). This does mean that there's a tangible change for all build file templates: they must now recognise and handle the `.o` extension, which is used internally to recognise object files internally. This extension was removed by common.tmpl before this change, but would mean that the platform specific templates wouldn't know if "foo.map" was originally "foo.map.o" (i.e. an object file in its own right) or "foo.map" (an export definition file that should be treated as such, not as an object file). For the sake of simplifying things, we also modify util/mkdef.pl to produce .def (Windows) and .opt (VMS) files that don't need additional hackery.
Remove some config attributes that just duplicate values that are already there in other attributes. Remove the special runs of mkdef.pl and mkrc.pl from build file templates, as these are now done via GENERATE statements in build.info. Remove all references to ordinal files from build file templates, as these are now treated via the GENERATE statements in build.info. Also remove -shared flags and similar that are there in shared-info.pl anyway. (in the case of darwin, it's mandatory, as -bundle and -dynamiclib don't mix)
clang on osx seems to currently fail on '%jd' specs, so turn off the check of printf format there.
levitte
force-pushed
the
getting-rid-of-Makefile.shared
branch
from
905024c
to
5e93223
Compare
|
I did a squash spree... I hope that's alright |
…braries Added note: We take the opportunity to clean out things that are redundant or otherwise superfluous (for example the check of GNU ld on platforms where it never existed).
|
I get a lot of linker warnings for aix-gcc, but make test passes... I haven't looked yet if it's specific to this request, or warnings were there all along... Checking aix-cc... |
|
Hmm... No warnings with aix-cc... |
|
aix-gcc warnings are not specific to this request, so that if they are to be fixed, it should be done separately... |
|
HP-UX work. [On side note, I'm not convinced that removal of equivalent of automatic LIBRPATH is justified universally. I mean it was removed, because it made some sense on Linux, but the argument was extended to all platforms, probably unjustified. So I'm likely to revisit it at some point...] |
| @@ -243,12 +243,12 @@ sub vms_info { | |||
| release => "-xO5 -xdepend -xbuiltin"), | |||
| threads("-D_REENTRANT")), | |||
| thread_scheme => "pthreads", | |||
| lflags => add("-xarch=generic64",threads("-mt")), | |||
| lflags => add(threads("-mt")), | |||
There was a problem hiding this comment.
Oh! So in other words lflags are also always used with cflags. Fair enough...
There was a problem hiding this comment.
Yup. If you look at Makefile.shared before this change, you will see that CFLAGS and LDFLAGS were always used when linking apps. Those are populated from cflags and lflags respectively
dot-asm
added
the
approval: done
…luator It will return the last expression from the input file. We also use this in read_config, which slightly changes what's expected of Configurations/*.conf. They do not have to assign %targets specifically. On the other hand, the table of configs MUST be the last expression in each of those files. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #4840)
This will replace the use of Makefile.shared This also means a small adjustment on how the attributes dso_cflags, dso_cxxflags and dso_lflags are treated. They were previously treated as an extension to shared_cflag, shared_cxxflag and shared_ldflag, but they should really be regarded as alternatives instead, for example for darwin, where -dynamiclib is used for shared libraries and -bundle for DSOs. We take the opportunity to clean out things that are redundant or otherwise superfluous (for example the check of GNU ld on platforms where it never existed). Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #4840)
This makes it possible to add build.info statements for using resource files as well as linker scripts (.def for Windows, .map for Unix, and .opt for VMS) is if they were source files. This requires changes in the build file templates. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #4840)
Because this also includes handling all sorts of non-object files when linking a program, shared library or DSO, this also includes allowing general recognition of files such as .res files (compiled from .rc files), or .def / .map / .opt files (for export and possibly versioning of public symbols only). This does mean that there's a tangible change for all build file templates: they must now recognise and handle the `.o` extension, which is used internally to recognise object files internally. This extension was removed by common.tmpl before this change, but would mean that the platform specific templates wouldn't know if "foo.map" was originally "foo.map.o" (i.e. an object file in its own right) or "foo.map" (an export definition file that should be treated as such, not as an object file). For the sake of simplifying things, we also modify util/mkdef.pl to produce .def (Windows) and .opt (VMS) files that don't need additional hackery. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #4840)
Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #4840)
Remove some config attributes that just duplicate values that are already there in other attributes. Remove the special runs of mkdef.pl and mkrc.pl from build file templates, as these are now done via GENERATE statements in build.info. Remove all references to ordinal files from build file templates, as these are now treated via the GENERATE statements in build.info. Also remove -shared flags and similar that are there in shared-info.pl anyway. (in the case of darwin, it's mandatory, as -bundle and -dynamiclib don't mix) Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #4840)
Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #4840)
Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #4840)
|
Thank you for the careful review |
Makefile.shared is a beast that we really should have dealt with for 1.1.0, but there wasn't time. It's a problematic procedure insofar that it hides away parts of what's actually happening and is very hard to debug. Better then to move the operation performed to the actual Makefile. While I'm at it, I've made the use of certain (undocumented) configs a bit more consistent with what's actually needed, and hopefully easier to read.
Checklist