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 a meson build system #12

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

Add a meson build system #12

wants to merge 3 commits into from

Conversation

1480c1
Copy link

@1480c1 1480c1 commented May 6, 2021

Allows for easy {cross-,}compilation for Windows and a lot of other systems

Adds a meson.build file that actually contains the directives for meson and the reversion.sh script written in basic python so it doesn't require a working sh

Also modifies unifdef.c to use <unifdef.h> so it's possible to use -I to influence where it decides to look for the header instead of looking relative to the file itself.

Currently, only supports building and installing unifdef$(EXE_SUFIX) and the manpages, but does not support the test, realclean, Changelong, unifdef.txt, and release targets yet as those will need additional work

to run a basic build,

meson builddir .
ninja -C builddir
sudo ninja -C builddir install

and that will produce ./builddir/unifdef and install /usr/local/bin/unifdef and the man pages

I am thinking of removing the sln file and the Makefile.mingw after I work on making sure there is nothing missing and after adding GitHub Actions

@fanf2
Copy link
Owner

fanf2 commented May 6, 2021

Thanks for this contribution!

There are a couple of things to address before it can be merged:

includes

The change to #include "unifdef.h" is a style bug: angle bracket includes should be reserved for system headers. The C standard says that quote includes search more places than angle includes, so -I on the compiler command line should work for all kinds of includes.

Was there some failure which prompted this change?

releases

The new reversion.py reminds me of a long-standing issue with releases on GitHub. They ship a bare zip of the repo working directory without generated source artefacts or the .git directory, so the build scripts (as currently written) can't work.

I plan to fix this for my various projects by changing the release process to create a release branch and commit the generated files to the branch. But of course that doesn't address the needs of CI for non-unix-like platforms so your reversion.py is heading in the right direction.

So that's OK so far, but I would prefer not to have two parallel implementations of the same thing, especially since the old shell one is fairly horrible. Since you are re-doing it in a Real Programming Language 😄 it would be better to completely remove reversion.sh and version.sh, and change reversion.py to pull the current version out of version.h. (version.sh is a hack to avoid writing a parser in a shell script)

You don't need to do anything else with the release machinery: that's just me thinking out loud and providing a bit of context.

@1480c1
Copy link
Author

1480c1 commented May 6, 2021

includes

The change to #include "unifdef.h" is a style bug: angle bracket includes should be reserved for system headers. The C standard says that quote includes search more places than angle includes, so -I on the compiler command line should work for all kinds of includes.

Was there some failure which prompted this change?

Using #include "unifdef.h" with unifdef.c being in the same location as the posix unifdef.h results in the compiler, always finding that one, even if I use -Iwin32 as the first line, causing the Windows builds to fail, now I can do the same exact thing the Makefile.mingw does and essentially make a copy of the file so the compiler no longer looks at the same directory of the file and instead looks through the include folders, however, that will cause meson to need to copy it every time the original file is updated and will cause Visual Studio to open the copy of the file instead of the original.

To elaborate, with the current setup, inside Visual Studio, clicking on unifdef.c will show the full path containing the absolute path to the source <repo>/unifdef.c, but using the alternative of copying the file will show <repo>/build/unifdef.c.

This means if I click on the file that shows up and make some changes/fixes etc. and hit recompile, I can see all of my changes being reflected in the compiled binary, however, the changes are made to a build specific copy and not the original, meaning when I go to do a git add unifdef.c and git commit, nothing is actually marked as changed

releases

The new reversion.py reminds me of a long-standing issue with releases on GitHub. They ship a bare zip of the repo working directory without generated source artefacts or the .git directory, so the build scripts (as currently written) can't work.

GitHub has a weird habit of wanting developers to work inside GitHub's weird system where if you want to make an actual "Release" and not just a snapshot of the repository at the tag, you would need to generate the artefacts yourself (git archive with autoreconfi -fiv and anything else) and then upload that to the "Release" page. For some people, it might be okay but from what I heard, a lot of other people do not really like GitHub's system.

So that's OK so far, but I would prefer not to have two parallel implementations of the same thing, especially since the old shell one is fairly horrible. Since you are re-doing it in a Real Programming Language smile it would be better to completely remove reversion.sh and version.sh, and change reversion.py to pull the current version out of version.h. (version.sh is a hack to avoid writing a parser in a shell script)

Sure, I can remove reversion.sh and rewrite part of reversion.py to instead pull it directly from version.h. I mainly left reversion.sh there so nothing breaks with the plain makefile

@1480c1
Copy link
Author

1480c1 commented May 6, 2021

For the includes issue, I can think of a few ways to resolve it, in no order in particular

  • Move unifdef.h to a subfolder like unix or posix and use -Iunix on the compilation line so that way we can use unifdef.c as it is in the master branch with " includes
  • Copy unifdef.c to the build directory and use that as the source instead (caveats noted in the previous comment)
  • Use it as it currently is in this PR
  • A final potential way is to use gcc's -I- option (gcc -I- -I win32 unifdef.c -o /dev/null forces gcc to look at win32/unifdef.h instead of the file's directory), but according to gcc's docs, it's depreciated and they recommend to use -iquote instead, but -iquote does not have the same desired effect. The obvious issue with this is that this is not portable and does not work with MSVC

Changes the Makefiles to add -Iunix for the normal Makefile and -Iwin32
for the mingw one. This makes it easier to change which unifdef.h gets
included in unifdef.c without needing to copy or move unifdef.c to the
build folder

Signed-off-by: Christopher Degawa <ccom@randomderp.com>
@1480c1
Copy link
Author

1480c1 commented May 9, 2021

For now, I will opt to use the Move unifdef.h to a subfolder like unix or posix and use -Iunix on the compilation line so that way we can use unifdef.c as it is in the master branch with " includes

I have removed reversion.sh and replaced references to it with the .py version and removed references to version.sh, although I would like some feedback on if the changes in scripts/release.sh is too ugly

leaving 3f31c69 so I have a breadcrumb in case I need to go back

1480c1 added 2 commits May 9, 2021 01:37
Simple usage example

```bash
meson build .
ninja -C build
```

Signed-off-by: Christopher Degawa <ccom@randomderp.com>
Also remove references to version.sh

Signed-off-by: Christopher Degawa <ccom@randomderp.com>
@1480c1
Copy link
Author

1480c1 commented Feb 24, 2022

ping @fanf2 this came up in my work again, so I wanted to revisit adding this again and potentially adding it to msys2's packages

@1480c1
Copy link
Author

1480c1 commented Feb 24, 2022

Is there anything specific this PR is missing, atm?

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.

2 participants