[PATCH] D54726: [libcxx] Support generating linker script for static library

[Louis Dionne via Phabricator]

ldionne added inline comments.


================
Comment at: libcxx/lib/CMakeLists.txt:388
+    ARGS
+      "${CMAKE_STATIC_LIBRARY_PREFIX}c++nonshared${CMAKE_STATIC_LIBRARY_SUFFIX}"
+      "$<TARGET_LINKER_FILE:cxx_static>"
----------------
I liked `c++static` more than `c++nonshared`, personally.


================
Comment at: libcxx/utils/gen_link_script.py:11
 
+import argparse
 import os
----------------
phosek wrote:
> ldionne wrote:
> > phosek wrote:
> > > ldionne wrote:
> > > > Can you confirm that there are no functional changes beyond adding support for a custom output file in this script? This is only a refactoring (thanks!), or did I miss something?
> > > The original script did not support creating a linker script for static library, so there's a functional change, but I also refactored the code a bit to make it (subjectively) simpler. I'd be fine doing the refactoring as a separate change if you prefer?
> > No, I'm fine with the refactoring + the functional change, I was just trying to spot what the functional change was. I'm sorry, but I still don't see what prevented the previous script from handling static archives. Can you please point it out?
> Previous version assumes that the input file (libc++) is a symlink which would be the case for `libc++.so` which a symlink to `libc++.so.1`. The script replace that symlink with a linker script that includes the library that the symlink was originally pointing to.
> 
> However, this doesn't work for static library which is a plain file `libc++.a`. The new version now handles input files that are not symlinks and it also allows renaming the input to output (the `--rename` option), so `libc++.a` would be first renamed to `libc++static.a` and generated linker script would include it as input.
> 
> Does this make sense?
Ok, thanks for explaining.


================
Comment at: libcxx/utils/gen_link_script.py:22
+                        action="store_true", default=False)
+    parser.add_argument("input", help="Path to libc++ library")
+    parser.add_argument("output", help="Path to libc++ linker script")
----------------
phosek wrote:
> ldionne wrote:
> > Please consider making these named parameters instead of positional parameters. That would have answered some of my questions before I had to ask them.
> I find it little awkward to use optional arguments as required arguments but I can do that if you prefer?
You can have required named arguments: `parser.add_argument('--foo', required=True)`. https://docs.python.org/2.7/library/argparse.html#required


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54726/new/

https://reviews.llvm.org/D54726