Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add option to disable unit tests #2459

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

cristeab
Copy link

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@cristeab
Copy link
Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@@ -150,22 +153,22 @@ if (${USE_STD_MAP} STREQUAL "OFF")
endif ()

# Add protoc (Protocol Buffers compiler) target.
set (RESOURCES_DIR "${CMAKE_SOURCE_DIR}/../resources")
set (RESOURCES_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../resources")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this variable name change specific to your setup?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please revert to old variable name?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMAKE_CURRENT_SOURCE_DIR is cmake specific and must be used in this context as it correctly specifies the current folder of the existing cmake file. CMAKE_SOURCE_DIR represents the current folder of the top level cmake file.

Copy link
Contributor

@penmetsaa penmetsaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Thanks for using libphonenumber and willing to contribute to the library.

Why do you want to have such an option? As we have not heared from others asking for this, we would like to know so that it helps in our review and get back to you ASAP. As there is no CPP expert (even for reviewing changes) within team, we are not actively adding features to build system unless absolutely necessary.

Some more feedback after my first look:

  • Could you please let me know why the protocol buffer files of JS are updated. If these are updated because of build and not related to your changes, please revert them.
  • Recieved below make error after pulling the edits. Am I missing anything here?
    [ 48%] Linking CXX static library libphonenumber.a
    [ 53%] Built target phonenumber
    Scanning dependencies of target geocoding_test_program
    [ 54%] Building CXX object CMakeFiles/geocoding_test_program.dir/test/phonenumbers/geocoding/geocoding_test_program.cc.o
    [ 55%] Linking CXX executable geocoding_test_program
    /usr/bin/ld: libphonenumber.a(phonenumberutil.cc.o): undefined reference to symbol 'pthread_once@@GLIBC_2.2.5'
    /usr/bin/ld: /lib/x86_64-linux-gnu/libpthread.so.0: error adding symbols: DSO missing from command line
    collect2: error: ld returned 1 exit status
    make[2]: *** [CMakeFiles/geocoding_test_program.dir/build.make:89: geocoding_test_program] Error 1
    make[1]: *** [CMakeFiles/Makefile2:188: CMakeFiles/geocoding_test_program.dir/all] Error 2
    make: *** [Makefile:152: all] Error 2
  • I have followed instructions of "Building and testing the library" at https://github.com/google/libphonenumber/tree/master/cpp

@@ -150,22 +153,22 @@ if (${USE_STD_MAP} STREQUAL "OFF")
endif ()

# Add protoc (Protocol Buffers compiler) target.
set (RESOURCES_DIR "${CMAKE_SOURCE_DIR}/../resources")
set (RESOURCES_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../resources")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please revert to old variable name?

option ("BUILD_UNIT_TESTS" "Build unit tests (gtest sources are needed)" "OFF")

if (ANDROID)
set (BUILD_GEOCODER OFF)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this?
If something is specific to your build, and may not be applicable or beneficiary for all, we can avoid that.

Copy link
Author

@cristeab cristeab May 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

geocoder is a binary for generating geo coding data. For cross-compilation builds, like Android, the binary should not be generated as it will not be run on Android. ANDROID variable comes from the toolchain cmake file provided within Android NDK.

Copy link
Author

@cristeab cristeab May 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are needed in order to allow libphonenumber to be integrated as subproject within another project using cmake and also to be able to cross-compile this library.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compilation errors you are seeing are likely linked to your environment. Try to use instead a Docker container. On my side I have compiled only on macOS

option ("USE_RE2" "Use RE2" "OFF")
option ("USE_STD_MAP" "Force the use of std::map" "OFF")
option ("BUILD_STATIC_LIB" "Build static libraries" "ON")
option ("BUILD_UNIT_TESTS" "Build unit tests (gtest sources are needed)" "OFF")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for common interest this should be by default "ON".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@penmetsaa
Copy link
Contributor

Hi,

As you might have understood by now, I am not familiar with CPP
Will try finding a CPP reviewer who has the context and expertise in build mechanics.
Thanks so much for your patience.

@silviucpp
Copy link

Any news related to this issue ? It's annoying that we are not capable to stop building unittests . In 99 % of the cases you don't need them. The only thing you need is the library to link with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants