lingua-franca icon indicating copy to clipboard operation
lingua-franca copied to clipboard

Add support for the `link-libs` target property

Open Soroosh129 opened this issue 2 years ago • 12 comments

Fixes #1120.

Soroosh129 avatar Apr 28 '22 18:04 Soroosh129

I'm not sure how to update the reference to the benchmarks to point here: lf-lang/benchmarks-lingua-franca#11

@lhstrh do you happen to know how this can be done?

Soroosh129 avatar Apr 28 '22 19:04 Soroosh129

Incidentally, the I actually do not understand why the LSP tests are failing in this branch. I tried to reproduce the Python validation test failure on my machine but could not, and instead, I had a test failure in Rust validation tests, which I should think would be unaffected by these changes. I thought that the random seed was set up so that this would not happen. I wonder if the LSP tests should be disabled temporarily until I find a way to make them less complex.

EDIT: The LSP test failures are no longer mysterious. I know not why the error went away.

petervdonovan avatar Apr 30 '22 05:04 petervdonovan

I would like to point out that there is already a field compileLibraries in TargetConfig. There appears to be a proliferation of competing mechanisms...

lhstrh avatar Apr 30 '22 22:04 lhstrh

One odd thing about this design is that m isn't, in fact, the library. The library is math. It's just that the common compiler flag to link it is m. I think that if we were to generalize, we should use the library names...

lhstrh avatar May 01 '22 02:05 lhstrh

Also, it appears that math is actually the only standard library that requires an extra flag (at least with gcc), so I think we've already created excess generality with this target property. It seems that a property link-math could possibly be sufficient. I'm now running tests to see what flags are really needed for standard libraries across the platforms we support.

lhstrh avatar May 01 '22 02:05 lhstrh

I think the only reasonable thing to do is to not make this a feature of 0.2.0 and first make sure this solution is solid. We can always publish a patch version that includes this feature.

lhstrh avatar May 01 '22 04:05 lhstrh

I think that handling special cases in this way is appropriate and could lead to an overall better user experience. Use of the math library is very common in C programs, and it really unfortunate that the it has to handled differently on different platforms. But once we understand those needs, why leave it users to have rediscover these quirks themselves?

The problem that I see is that this knowledge base will never be complete, and it needs to be maintained (as libraries and their usage change over time). If we promise users that they can just specify the library they want to link to and it works on any platform, then we need to fulfill this promise somehow. The effort required to do this seems extensive to me. If we really want to go down that path, we should at least be transparent about precisely which libraries are supported. We should have tests for every library we claim to support and issue validation errors (or at least warnings) if users try to use the target property for linking other libraries.

cmnrd avatar May 02 '22 07:05 cmnrd

I see that there has been a lively discussion here in my absence. I apologize for my late response.

In terms of a special handling for libm, the unfortunate situation is that some systems require the -lm flag and some don't. This goes back to older days where some implementations of the C standard library (libc) separated out the math functions' implementation into a separate library. Newer implementations of libc seem to already incorporate the math functions, so there is no need to link against libm. Most newer compilers, including GCC and Clang, just ignore the -lm if you pass it, but, unfortunately, the MSVC compiler (which is a compiler that we support) patently refuses to compile if libm is linked.

If you want to use the math functions in your code and ensure maximum compatibility, you need to add the following to your cmake-includes:

IF (NOT WIN32)
  find_package(m REQUIRED)
  target_link_libraries(${LF_MAIN_TARGET} m)
ENDIF()

The math library is really an oddity in the C standard library. As far as I know, there is no other standard library for C that needs to be specifically linked. Hence, I think it was justified to provide an easy shortcut for this in the form of link-libraries: ["m"], as suggested originally in #1120. An alternative would be to provide a built-in mlib.cmake to allow for cmake-include: ["mlib.cmake"].

In terms of the usefulness of link-libraries in general, it is really a shorthand for the following:

  • If cmake: false, pass the specified libraries via the -l flag.
  • If cmake: true, pass the specified libraries via
    find_library( LF_FOO_LIB foo )
    target_link_libraries( ${LF_MAIN_TARGET} "${LF_FOO_LIB}")
    

This is providing a shorthand for a scenario that can arise many times. Note that this mechanism supports cmake: false as well. Of course, cmake-include will always be there if a more advanced situation arises (although, you would have to leave cmake: false behind). Think of link-libraries as the easy mode for a common pattern.

Finally, I envisioned link-libraries as a method that also allows for inclusion of our own utility libraries (e.g., link-libraries: ["deque"]), but this could wait until a future PR.

I see that the the link-libraries is renamed to link-std-libs. This sounds wrong to me. I think if we want to keep it, the link-libraries name sounded more appropriate.

Soroosh129 avatar May 23 '22 10:05 Soroosh129

My suggestion would be to wait with finalizing this until @cmnrd returns from his vacation so we can reach agreement on what the final solution should look like.

lhstrh avatar May 23 '22 19:05 lhstrh

Tagging @cmnrd now that he's back to see if we can reach a consensus. @cmnrd, please see my comment above.

Soroosh129 avatar Jun 20 '22 15:06 Soroosh129

With regard to the math library, why not add -lm by default? Is there a case where we really don't want to add the math library?

Ok, so if we see link-libraries as a shortcut for a specific piece of cmake code, then I think it is reasonable to include it. We should then specifically describe in the documentation which cmake code it produces, in which scenarios it works, and how the user can add manual cmake code when it doesn't work. From the initial proposal, I understood link-libraries as a general solution to the problem of finding and linking dependencies, which I don't think it is possible. If we label this as a limited solution that works in some (or many?) cases, I am fine with including it.

cmnrd avatar Jun 21 '22 07:06 cmnrd

The c-benchmark tests will fail because lf-lang/benchmarks-lingua-franca#11 is not merged yet. I have no idea how to proceed. Could you help me @lhstrh, please?

Soroosh129 avatar Jun 26 '22 02:06 Soroosh129