Skip to content
This repository has been archived by the owner on Nov 9, 2022. It is now read-only.

Libs so #6

Merged
merged 12 commits into from
Feb 10, 2018
Merged

Libs so #6

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
22762f9
Added changes to Makefiles to create shared object
cyocum Jan 23, 2018
65ab06d
Made libgherkin.so build by default
cyocum Jan 23, 2018
b974507
Removed unnecessary linker directive
cyocum Jan 23, 2018
b0ade5f
Created a make install and moved file
cyocum Jan 23, 2018
621dd46
Attempting to fix error on CI with mingw-gcc
Jan 25, 2018
8325fb2
Made changes based on reviewer feedback
Jan 26, 2018
7a63949
Merge branch 'master' into libs_so
aslakhellesoy Jan 26, 2018
9e73887
Removed -soname from shared flags as this was causing failure on OSX
Jan 26, 2018
9ca47fa
Added libs_so to all target
cyocum Jan 29, 2018
05c5675
Correct the target of the make rule for the dynamic library.
brasmusson Jan 29, 2018
9f1cd4d
gherkin: Add .PHONY targets, formatting
aslakhellesoy Jan 30, 2018
0bd23be
Merge branch 'master' into libs_so
aslakhellesoy Jan 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,22 @@ libs: ./include/rule_type.h src/parser.c src/dialect.c $(SRC_FILES) src/Makefile
cd src; $(MAKE) CC=$(CC) $@
.PHONY: libs

libs_so: ./include/rule_type.h src/parser.c src/dialect.c $(SRC_FILES) src/Makefile
cd src; $(MAKE) CC=$(CC) $@
.PHONY: libs_so

install: libs_so
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that the install and uninstall should be part of this makefile and make assumptions on where to install. If so the install and include directory should be defined in variables, so the they can be edited in one place (or overridden from the command line).

Copy link
Contributor

Choose a reason for hiding this comment

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

If there are install and uninstall rules, then shouldn't they also install the "bin/gherkin" CLI-binary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. This was requested by @aslakhellesoy because of things I said in the README file for Cucumber.ml project. I am not wedded to it because I have not used make in years and am (as you can see) fairly rusty at it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove install and uninstall, but I thought having them would simplify consumption for other developers and users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hardcoding things like "/usr/lib/" on several lines deep in makefile rules is not a good idea. The src/Makefile is deliberately written with make variable at the top, so that is should be straight forward for a user to modify those make variables for the special c compiler that the user might use. Any install/uninstall rules should also be appropriately parameterized ("/usr/lib/" does not work on Windows for instance).

cp ./libs/libgherkin.so.1.0 /usr/lib/libgherkin.so.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

.1.0? Shouldn't it rather be the gherkin version?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we add a VERSION file with a single line containing the semver version. The upcoming version is 5.0.1. Then the Makefile can just cat that file.

ln -s /usr/lib/libgherkin.so.1.0 /usr/lib/libgherkin.so
mkdir -p /usr/include/gherkin
cp ./include/*.h /usr/include/gherkin
.PHONY: install

uninstall:
rm /usr/lib/libgherkin.so /usr/lib/libgherkin.so.1.0
rm -rf /usr/include/gherkin
.PHONY: uninstall

.run: cli $(GHERKIN) $(GOOD_FEATURE_FILES)
$(RUN_GHERKIN) $(GOOD_FEATURE_FILES) | jq . > /dev/null
touch .run
Expand Down Expand Up @@ -96,3 +112,4 @@ acceptance/testdata/%.feature.source.ndjson: testdata/%.feature testdata/%.featu
mkdir -p `dirname $@`
$(RUN_GHERKIN) --no-ast --no-pickles $< | jq --sort-keys --compact-output "." > $@
diff --unified <(jq "." $<.source.ndjson) <(jq "." $@)

0 src/error_list.h → include/error_list.h
View file
Open in desktop
File renamed without changes.
0 src/file_reader.h → include/file_reader.h
View file
Open in desktop
File renamed without changes.
16 changes: 12 additions & 4 deletions src/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
GCC_FLAGS=-c -Wall -Werror -g
CLANG_FLAGS=-c -Wall -Werror -g
GCC_FLAGS=-c -Wall -Werror -g -fPIC
CLANG_FLAGS=-c -Wall -Werror -g -fPIC
GCC_SO_FLAGS= -shared -Wl,-soname,libgherkin.so.1
Copy link
Contributor

Choose a reason for hiding this comment

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

".1"? not ".1.0" as everywhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or better - take the version from a VERSION file as described above

MINGW_FLAGS=-c -Wall -Werror -g

ifeq ($(CC),i686-w64-mingw32-gcc)
CC=i686-w64-mingw32-gcc
LD=i686-w64-mingw32-gcc
CC_FLAGS=$(GCC_FLAGS)
CC_FLAGS=$(MINGW_FLAGS)
AR=i686-w64-mingw32-ar
EXT=.exe
else ifeq ($(CC),clang)
Expand Down Expand Up @@ -33,6 +35,8 @@ cli: ../bin/gherkin$(EXT)

libs: ../libs/libgherkin.a

libs_so: ../libs/libgherkin.so

clean:
$(RM_CMD) ../bin ../objs ../libs
.PHONY: clean
Expand Down Expand Up @@ -121,10 +125,14 @@ GHERKIN_CLI_OBJS= \
$(MKDIR_CMD) $(dir $@)
$(AR) $(AR_FLAGS) $@ $(UTILITIES_OBJS) $(PARSER_OBJS) $(AST_OBJS) $(COMPILER_OBJS) $(PICKLES_OBJS) $(EVENT_OBJS)

../libs/libgherkin.so: $(UTILITIES_OBJS) $(PARSER_OBJS) $(AST_OBJS) $(COMPILER_OBJS) $(PICKLES_OBJS) $(EVENT_OBJS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless declared ".PHONY", make rules shall create their target, so the make target should be "../libs/libgherkin.so".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not understand what you mean (again, my make experience is not great) so I am unable to comply. If you can make the change so I can see what you mean, that would be very helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

"A phony target is one that is not really the name of a file". "../libs/libgherkin.so" is "not really the name of a file" (the name of the file is "../libs/libgherkin.so.$(VERSION)". Since the basic principle of make is that the target of a rule is a file (unless there is a very compelling reason otherwise - for instance for rules with the target "all"), "../libs/libgherkin.so.$(VERSION)" is a much better target for this rule. It also enable the use of the automatic variable $@

$(MKDIR_CMD) $(dir $@)
$(CC) $(GCC_SO_FLAGS) -o ../libs/libgherkin.so.1.0 $(UTILITIES_OBJS) $(PARSER_OBJS) $(AST_OBJS) $(COMPILER_OBJS) $(PICKLES_OBJS) $(EVENT_OBJS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use "-o $@" (after the make target has been fixed).


../bin/gherkin_generate_tokens$(EXT): ../libs/libgherkin.a $(GENERATE_TOKEN_OBJS) Makefile
$(MKDIR_CMD) $(dir $@)
$(LD) $(LD_FLAGS) $(GENERATE_TOKEN_OBJS) -L../libs/ -lgherkin $(LD_LIBS) -o $@

../bin/gherkin$(EXT): ../libs/libgherkin.a $(GHERKIN_CLI_OBJS) Makefile
../bin/gherkin$(EXT): ../libs/libgherkin.a ../libs/libgherkin.so $(GHERKIN_CLI_OBJS) Makefile
Copy link
Contributor

Choose a reason for hiding this comment

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

"../libs/libgherkin.so" is not used for building "../bin/gherkin$(EXT)", so it should not be a dependency of the make rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I do not understand. ../libs/libgherkin.so does build its target because it calls CC to build the actual shared object so I do not know what to do to address your comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

The "../bin/gherkin" binary is statically linking the gherkin library. It does so using by using the "../libs/libgherkin.a" archive file. When you execute make ../bin/gherkin, "../libs/libgherkin.so" should not be built because it is not needed/used to produce "../bin/gherkin".

I think you added this to get "../libs/libgherkin.so" build when running make (with no argument), but this is the wrong way to achieve that (introducing a fake dependency to a make rule). To achieve that "../libs/libgherkin.so" build when running make, the "all" rule should be changed.

$(MKDIR_CMD) $(dir $@)
$(LD) $(LD_FLAGS) $(GHERKIN_CLI_OBJS) -L../libs/ -lgherkin $(LD_LIBS) -o $@