Libart-lgpl moved into a 'src' folder for consistency sake with the other #2

Merged
MicheleC merged 1 commits from feat/new_src_layer into master 4 years ago
Ghost commented 4 years ago

modules.
Add basic cmake build instructions.
Rework of the README file.
Some cosmetics.

modules. Add basic cmake build instructions. Rework of the README file. Some cosmetics.
MicheleC reviewed 4 years ago
MicheleC left a comment
Owner

Other than the comment below, it seems good. I did a quick build test and it works, but have not yet done a comparison of the build with the previous versions. Will check that tomorrow.

Other than the comment below, it seems good. I did a quick build test and it works, but have not yet done a comparison of the build with the previous versions. Will check that tomorrow.
##### check for the math libc
find_library( MATH_LIBC m )
if( MATH_LIBC )
Owner

Am I missing something or this

set( MATH_LIBC "${MATH_LIBC}" )

just set the variable to itself?

In such case how about something like this:

if( NOT MATH_LIBC )
  tde_message_fatal( "libm library is required, but not found on your system" )
endif( )

libm.so is part of libc6 (in debian at least), so most likely the error message should never show up.

Am I missing something or this ``` set( MATH_LIBC "${MATH_LIBC}" ) ``` just set the variable to itself? In such case how about something like this: ``` if( NOT MATH_LIBC ) tde_message_fatal( "libm library is required, but not found on your system" ) endif( ) ``` libm.so is part of libc6 (in debian at least), so most likely the error message should never show up.
obache commented 4 years ago
Collaborator

Some systems are missing libm, because any math functions in libc like system default library.

Some systems are missing libm, because any math functions in libc like system default library.
Owner

Setting set( MATH_LIBC "${MATH_LIBC}" ) is unnecessary because it does not make sense to set a variable to a value of itself. This would only make sense if it was associated with setting the value to the cmake cache. But the cache setting is already done in find_library.

I think m is one of the libraries that can be either standalone or part of the basic libc. Therefore, its absence may not be a problem. Here the question is whether we know which functions it concerns and whether we can test for the presence of such functions in standard libc.

Setting `set( MATH_LIBC "${MATH_LIBC}" )` is unnecessary because it does not make sense to set a variable to a value of itself. This would only make sense if it was associated with setting the value to the cmake cache. But the cache setting is already done in `find_library`. I think `m` is one of the libraries that can be either standalone or part of the basic libc. Therefore, its absence may not be a problem. Here the question is whether we know which functions it concerns and whether we can test for the presence of such functions in standard libc.
Owner

The original code was linking against libm (hardcoded). I think for the purpose of this PR we can rework the part above and silently ignore it if MATH_LIBC is not set. Later we can look at which function is required from libm, but we can separate this from this PR, IMO.

What do you think?

The original code was linking against libm (hardcoded). I think for the purpose of this PR we can rework the part above and silently ignore it if MATH_LIBC is not set. Later we can look at which function is required from libm, but we can separate this from this PR, IMO.<br/> What do you think?
Owner

Yes, a solution with libm detection is better than with hardcoded linking. Silently ignoring the absence of libm does not seem to be a problem. And if needed, it can be improved later. Therefore, I agree with the state of PR as it is now.

Yes, a solution with libm detection is better than with hardcoded linking. Silently ignoring the absence of libm does not seem to be a problem. And if needed, it can be improved later. Therefore, I agree with the state of PR as it is now.
Owner

👍

We just need Greg to get rid of that if() and we can merge .

:+1:<br/> We just need Greg to get rid of that if() and we can merge .
Owner

I compared the build from master and from the new branch. It looks ok.

I compared the build from master and from the new branch. It looks ok.
Ghost changed title from Libart-lgpl moved into a 'src' folder for consistency sake with the other to WIP:Libart-lgpl moved into a 'src' folder for consistency sake with the other 4 years ago
Ghost added the PR/wip label 4 years ago
Ghost removed the PR/wip label 4 years ago
Ghost changed title from WIP:Libart-lgpl moved into a 'src' folder for consistency sake with the other to Libart-lgpl moved into a 'src' folder for consistency sake with the other 4 years ago
MicheleC approved these changes 4 years ago
MicheleC left a comment
Owner

Looks good to me now.

Looks good to me now.
SlavekB approved these changes 4 years ago
SlavekB left a comment
Owner

It looks good.

It looks good.
MicheleC merged commit 8349a964c2 into master 4 years ago
MicheleC deleted branch feat/new_src_layer 4 years ago
MicheleC added this to the R14.0.9 release milestone 4 years ago
Owner

Thanks Greg!

Thanks Greg!

Reviewers

MicheleC approved these changes 4 years ago
SlavekB approved these changes 4 years ago
The pull request has been merged as 8349a964c2.
Sign in to join this conversation.
No Milestone
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/libart-lgpl#2
Loading…
There is no content yet.