Skip to content
This repository has been archived by the owner on Mar 22, 2023. It is now read-only.

tests: set umask for libpmemfile tests #217

Merged
merged 1 commit into from
Jun 23, 2017

Conversation

marcinslusarz
Copy link
Contributor

@marcinslusarz marcinslusarz commented Jun 22, 2017

Right now we depend on umask being set to 022 in the environment.


This change is Reviewable

@@ -38,6 +38,12 @@ add_cstyle(tests-preload-config ${CMAKE_CURRENT_SOURCE_DIR}/config/config.c)
add_check_whitespace(tests-preload-basic ${CMAKE_CURRENT_SOURCE_DIR}/basic/basic.c)
add_check_whitespace(tests-preload-config ${CMAKE_CURRENT_SOURCE_DIR}/config/config.c)

add_library(setumask_o OBJECT setumask.c)
Copy link
Contributor

@GBuella GBuella Jun 22, 2017

Choose a reason for hiding this comment

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

I think there is no need for an intermediary OBJECT target for this.
Neither is the _shared postfix needed, as we don't build a static version of this.
Also, I don't see why we would need to set OUTPUT_NAME.
It could just:

add_library(setumask SHARED setumask.c)
set_target_properties(setumask PROPERTIES INCLUDE_DIRECTORIES ${CMAKE_SOURCE_DIR}/src/)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,9 @@
#include "compiler_utils.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't check_license say something about missing license here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@krzycz
Copy link
Contributor

krzycz commented Jun 22, 2017

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

add_library(setumask SHARED setumask.c)
set_target_properties(setumask PROPERTIES
INCLUDE_DIRECTORIES ${CMAKE_SOURCE_DIR}/src/
POSITION_INDEPENDENT_CODE ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does one really need to manually set POSITION_INDEPENDENT_CODE for a lib marked as SHARED? Isn't that automatic?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that is needed when adding an OBJECT library, as cmake doesn't know yet where that is going to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It failed on Travis without it. See https://travis-ci.org/pmem/pmemfile/builds/245748025

Copy link
Contributor

Choose a reason for hiding this comment

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

That travis build doesn't correspond to what is here, that still contains the extra OBJECT target.

Scanning dependencies of target setumask_o
[ 61%] Building C object tests/preload/CMakeFiles/setumask_o.dir/setumask.c.o
[ 61%] Built target setumask_o
...
[ 71%] Linking C shared library libsetumask.so
/usr/bin/ld: CMakeFiles/setumask_o.dir/setumask.c.o: relocation R_X86_64_PC32 against undefined symbol `umask@@GLIBC_2.2.5' can not be used when making a shared object; recompile with -fPIC

So of course, while compiling the object target, cmake didn't know if that should be PIE or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@codecov-io
Copy link

codecov-io commented Jun 22, 2017

Codecov Report

Merging #217 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
+ Coverage    78.8%   78.82%   +0.02%     
==========================================
  Files          70       71       +1     
  Lines       11016    11019       +3     
  Branches     1493     1493              
==========================================
+ Hits         8681     8686       +5     
- Misses       1813     1815       +2     
+ Partials      522      518       -4
Impacted Files Coverage Δ
tests/preload/setumask.c 100% <100%> (ø)
src/libpmemfile-posix/file.c 65.4% <0%> (ø) ⬆️
src/libpmemfile-posix/rename.c 94.39% <0%> (ø) ⬆️
src/libpmemfile-posix/dir.c 82.41% <0%> (+0.29%) ⬆️
src/libpmemfile-posix/inode.c 90.17% <0%> (+0.42%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fcfceb...860cce3. Read the comment docs.

Right now we depend on umask being set to 022 in the environment.
@GBuella
Copy link
Contributor

GBuella commented Jun 22, 2017

:lgtm:


Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@sarahjelinek
Copy link

:lgtm:


Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@sarahjelinek sarahjelinek merged commit d518fa1 into pmem:master Jun 23, 2017
@marcinslusarz marcinslusarz deleted the umask branch June 27, 2017 13:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants