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 CMake build script #5

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

Add CMake build script #5

wants to merge 2 commits into from

Conversation

dbrgn
Copy link

@dbrgn dbrgn commented Oct 2, 2020

Hi, thanks for your library! I help out with the LibrePCB project (https://github.com/librepcb/librepcb) which is using your library for serializing and parsing library elements.

Right now we link statically against your library (using a qmake config added here in a fork), but slowly distro maintainers are starting to package LibrePCB. And distro maintainers don't like statically linked libraries 🙂

So I started thinking about packaging your library for Arch Linux, in order for LibrePCB to be able to link dynamically against it. However, without a build system this is a bit tricky. So here's a CMake configuration that allows building and installing your library easily.

Note: I don't really have much experience with CMake so far, I based the configuration on this SO answer which seems pretty nice. I picked CMake because it is widely used and pretty simple to use when packaging projects into Linux distros.

Note 2: The setup also generates a pkg-config file. This is very useful to be able to consume the library. For example, in qmake (which is used a lot in Qt projects), I could simply link against sexpresso by using PKGCONFIG += sexpresso, without having to set up include paths myself.

CC @ubruhin

@BitPuffin
Copy link
Owner

Hello! Thanks for making the pull request and getting in touch.

I am very excited to see my library being used in a cool project like this. Was not expecting that!

At first I was reluctant to merge this because i don't really use CMake so I wouldn't wanna figure out how to maintain the project with it. But since the project is unlikely to grow it should probably be ok. I have however been planning to convert the library to a single header library. Nevertheless, that shouldn't prevent it from being possible to build it into a library for package maintainers.

I will test out that the CMake build files work and then merge when I have time!

Again thanks!

@BitPuffin
Copy link
Owner

Ah, I saw that the project replaced it with its own parser, which is fair enough! 😃

However I think I'm gonna try to address the issues that caused the removal, not that I think that they'll take the library back because of it. But it seems like the library would be more useful for future projects if I can fix them. Creeping the issue tracker from what I can gather the problems are:

  • The packaging / unbundling issue that you have provided a solution for here (thanks!)
  • Not good enough for serialization / didn't meet the project's needs. (although the issue I read from was a bit vague. Just said they wanted control the line breaks etc, I wonder if I can provide that in a neat way)
  • sexpresso::escape doesn't work like desired (escapes a few weird characters [not sure why I did that lol], easy fix).
  • Could be faster (probably not a huge deal, but faster is nicer).

If there is anything I've missed or other ways to improve the library I would really appreciate feedback!

@dbrgn
Copy link
Author

dbrgn commented Nov 21, 2020

Hi, thanks for the feedback!

Ah, I saw that the project replaced it with its own parser, which is fair enough!

Yeah, because we only used it as parser, not as generator, so it made sense to customize the parser as well 🙂

Maybe @ubruhin can chime in and give some feedback?

@ubruhin
Copy link

ubruhin commented Nov 21, 2020

Hi @BitPuffin,

First of all, thanks for this library! Although now we don't use it anymore, in the beginning it helped a lot to get started quickly with S-Expression files. And at least the parser was part of LibrePCB for several years. 🙂

Creeping the issue tracker from what I can gather the problems are:

  • The packaging / unbundling issue that you have provided a solution for here (thanks!)
  • Not good enough for serialization / didn't meet the project's needs. (although the issue I read from was a bit vague. Just said they wanted control the line breaks etc, I wonder if I can provide that in a neat way)

Yes, basically our own generator implementation allows to choose at which position the linebreaks are inserted and what indentation is used (we want 1 space per nesting level). This way we can get nice looking, well structured files to make them more suitable for version control systems since the diff is easier to read. See this blog post for details, and how our generated S-Expression files look like.

Of course this limitation doesn't apply only to sexpresso. Most of the existing serialization libraries - no matter what file format - do not allow to control the formatting details of the generated output.

sexpresso::escape doesn't work like desired (escapes a few weird characters [not sure why I did that lol], easy fix).

Well, I think that's a matter of missing specifications for the S-Expression file format, so we can do escaping how we like it 😉 And for the same reason as above (more readable files and diffs), we removed unnecessary escaping (' is easier to read than \\').

Could be faster (probably not a huge deal, but faster is nicer).

This was not really a criticism to sexpresso, I just saw that the own implementation was faster than using sexpresso. But that doesn't mean sexpresso is slow. The thing is that for LibrePCB we use Qt, therefore we had to convert between raw C++ types and Qt types (e.g. sexpresso uses std::string, LibrePCB uses QString). Since our own implementation is now written directly using Qt types, the conversion is no longer needed, so it's kind of obvious that we can get better performance 😉

@ubruhin
Copy link

ubruhin commented Nov 21, 2020

Ah and btw, our own generator also clearly distinguishes between "token" and "string". Tokens will not get surrounded by quotes (and therefore must not contain spaces) while strings are always surrounded by quotes (no matter if they contain spaces or not). This leads to a more self-explaining file format (you can guess the actual data type of the values) and to smaller diffs (not introducing/removing quotes if spaces are added/removed). Example:

(stroke_text 0523853e-390b-480d-80f0-22df28be1042 (layer bot_names)
 (height 1.0) (stroke_width 0.2) (letter_spacing auto) (line_spacing auto)
 (align center bottom) (position 56.92125 71.12) (rotation -270.0)
 (auto_rotate true) (mirror true) (value "NAME")
)

Without knowing the details of the fileformat, one can guess that stroke_text is some kind of object name, 1.0 is a number, true is a boolean, but NAME is a string. This also allows some basic syntax highlighting of such files.

@BitPuffin
Copy link
Owner

BitPuffin commented Nov 22, 2020

@ubruhin Thanks for taking the time to write up some feedback! Very much appreciated. Also, LibrePCB is a very very cool project! If I ever get around to looking into PCB design it will be on my shortlist for things to use.

It is great that the library was useful in the beginning. I would expect a big project to eventually outgrow it though in its current state when more advanced functionality becomes desirable. I think I wanna revise the API a bit so that it makes simple things easy to do (like they more or less are now) and more advanced things possible (even if it's somewhat clunky, it will still be less work to use than having to replace the parser entirely).

For example choosing sane defaults for an escape function that you can use directly, but being able to provide a list of customized chars if desired. Or maybe being able to provide some kind of table to the serialization function that lets you do the format customization you ended up implementing yourselves.

Also yeah the std::string -> QString conversion would cause some overhead, so that immediately explains the performance gains. Although I should probably still check if there are some places to improve the performance without too much complexity. I wasn't really expecting someone to use the library except for maybe friends so I haven't been entirely rigorous in benchmarking.

One thing I still find regrettable since its creation is that it's a bit tedious to have to write serialization functions for every type you wanna serialize. It's kind of hard to give a reasonable solution to serialization in C++ because it is a bit of a mess when it comes to introspection. But maybe some kind of separate tool that integrates with Sexpresso for generating serialization would be nice.

Ah and btw, our own generator also clearly distinguishes between "token" and "string"

Yeah I think that's a good choice. I was trying to keep things as simple as possible when I made sexpresso but over time I have come to regret my decision in regards to not having a symbol/token type. Having them be separate types embeds the data with a bit more meaning. Beyond that, perhaps having separate number types as well would be a nice addition.

Again thanks for the feedback! I'll keep an eye on LibrePCB :)

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