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

merged 12 commits into from Feb 10, 2018

Conversation

cyocum
Copy link
Contributor

@cyocum cyocum commented Jan 24, 2018

This pull request amends the Makefiles for the project to allow for the creation of a libgherkin.so file which can be installed on a Unix/Linux machine and integrated in other projects. This is necessary for cucumber.ml.

  • Moved error_list.h and file_reader.h from src into the include directory so they can be used by other programs
  • Created the lib_so make target so that it will build the libgherkin.so file
  • Created the ../libs/libgherkin.so make target so that it will build the libgherkin.so file
  • Created the install and uninstall targets so that the user can install the created library and include files into the current system so that it can be compiled against in other projects.

cyocum and others added 5 commits January 23, 2018 18:25
* Added changes to Makefiles to create shared object
* Moved file_reader.h to include file so it can be used
  in the library.
* Created a make install target based on Unix conventions
* Moved error_list.h as it is required by a include/ file
Makefile Outdated
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).

Makefile Outdated
.PHONY: libs_so

install: libs_so
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.

src/Makefile Outdated
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

src/Makefile Outdated
@@ -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 $@

src/Makefile Outdated
@@ -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)
$(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).

src/Makefile Outdated
../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.

Chris Yocum and others added 2 commits January 26, 2018 10:11
* Added VERSION file
* Removed make install/unintstall
* Added cat'ing the VERSION file to create the libgherkin file name
@aslakhellesoy
Copy link
Contributor

@cyocum I changed the CI config so the build runs on OS X as well as Linux. That exposed some more issues that I also saw on my local Mac.

@cyocum
Copy link
Contributor Author

cyocum commented Jan 26, 2018 via email

@brasmusson
Copy link
Contributor

It seems that on Mac -install_name is the equivalent of -soname https://stackoverflow.com/a/8424297

@aslakhellesoy
Copy link
Contributor

@brasmusson @cyocum what's left to do before I can merge this? The build is green.

@cyocum
Copy link
Contributor Author

cyocum commented Jan 29, 2018

@aslakhellesoy I need to address @brasmusson comment above concerning the "all" make rule. After that, I think we are done unless @brasmusson has any other things he needs.

@aslakhellesoy
Copy link
Contributor

I've added .PHONY targets, so I think this is good to merge now?

@brasmusson
Copy link
Contributor

I think it is good to merge now.

@cyocum
Copy link
Contributor Author

cyocum commented Feb 9, 2018 via email

@aslakhellesoy aslakhellesoy merged commit ccb685f into master Feb 10, 2018
@aslakhellesoy
Copy link
Contributor

Hi @cyocum,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

  • ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
  • 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
  • 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
  • ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
  • 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

@aslakhellesoy aslakhellesoy deleted the libs_so branch February 10, 2018 13:39
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Sep 23, 2018
5.1.0 - 2018-05-30

Added

* (.NET) - Better .NET Core support
* Support for Aragonese (#298 danilat)
* (C) build a shared libgherkin.so library which allows Gherkin to be used as
  a library. (Cucumber.ml currently uses this.) (cucumber/gherkin-c#6 cyocum)

Changed

* Pass the content type of a docstring down into its pickle string form (#292
  rjwittams)
* Fixed Russian equivalents of Given and Then. (#369 cerebellum13)

Fixed

* (C) Segfault when file does not exist (#394 #395 cyocum)
* (JavaScript) (#374 #377 charlierudolph)
* (Ruby, JavaScript) Remove berp.exe from packages (#289 aslakhellesoy)
* (Go) fixes validation for go vet tool on latest versions (#330 l3pp4rd)
* (Ruby) removed unneeded files from the gem
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.

None yet

3 participants