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

Make ESP build with Bazel #52

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

Conversation

davidgiven
Copy link
Contributor

@davidgiven davidgiven commented Jan 28, 2019

I'm bored at an airport, so here's an unfinished snapshot of a system to build ESP (and dependent libraries) with Bazel. It's mostly intended for comments and a way to start unpicking the source code rather than a serious proposal to include it (although as a build system Bazel is awesome, and way more suited for correct, fast, complex builds than make). It works, but it's full of warnings. Do this to build:

bazel build //Tools/esp

Things for comment:

  • I would be very much inclined to just drop all the Tools/include/compat stuff --- systems these days are much more standardised than they were back then, and it's just complex and problematic. I spent a while trying to, for example, unpick compat/assert.h so that I wouldn't get an implicit definition warning on assert and failed.

  • There are 64-bit violations everywhere --- casting pointers to ints and back again. I've changed some of them to intptr_t, but I'm sure there's some I've fixed. I'm particularly suspicious of the Tools/utils/malloc.c. Can this just be replaced with the system malloc? They used to be terrible, and back in the day the place I worked would routinely replace the system malloc with a custom, much faster one, but not any more.

  • There are a lot of generated files which are checked in; parse.[ch] are built with yacc from parse.y. I've removed parse.[ch] and invoke as part of the build --- there should only be a single source of truth for any source file.

  • ESP uses tables generated by gperf a lot, and the source gperf files are missing, and the generated source uses obsolete inline semantics, which involved hacking to work around. Ideally we need new gperf files and then the build system generates the tables on the fly. (Recreating the gperf files shouldn't be hard.)

  • Warnings! Everywhere! I fixed a bunch, but there's way more to go.

  • I fixed a genuine bug! The scanner code to detect the start of strings is wrong --- it does c=='"' || c=='\'' && !in_string, to try and detect a quotation mark when it's not in a string, except && has a higher priority than ||. I actually think I remember finding this bug in GOC way back in the day.

  • The source really ought to be run through clang-format... is there a standard coding style?

@jirkakunze
Copy link
Collaborator

Your work on ESP looks very good. It would be very nice to have a more modern building tool.

@bluewaysw
Copy link
Owner

Thank you for this inspiring PR! Some thoughts back:

Our current approach in the repo is to make it build with changing only things that are necessary to change. A next step later should be a more in deep refactoring, then we should get rid of the outdated compatibility layers.

Yes, 64bit was out of scope back the time the code has been written. I think switching to 64bit will require a lot of fixing and debugging. malloc likely would be an more easy part to replace.

Having one source of truth would be valuable, I would need to check if we have the gperf sources somewhere. But actually switching to make bison and gperf run as part of the standard build also increases the pre-requirements of tool to be available to make the build work. We especially target Windows and Linux compatible build, introducing this goal would make the tools setup more diverse. Futhermore we need to double check if we have to backport changes we did on the checked in result files.

Yes, there are also some warning left, also when building using WATCOM but not too much, we should fix them asap.

The bugfix seems interesting, thx for it! We should integrate this one soon.

As for formating: They applied a set of formating rules manually I would say. We figured out that the code formating is best using real TAB with 8 char width, but indention to be 4 chars. This ends up in a funny mix of spacing and TAB used but this delivers a good readable layout of most of the orignally code, some newer change might have broken the rules already.

Our current goal is to make PC/GEOS build under Linux and Windows using WATCOM, replacing some part that are not free. Once this is done we consider a deeper refactoring/restructuring, may be that we end up with bazel as an option, this is very interesting. Keep pushing changes and thoughts in this regard if you like!

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

Successfully merging this pull request may close these issues.

3 participants