-
Notifications
You must be signed in to change notification settings - Fork 543
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
CMake #378
CMake #378
Conversation
#cmakedefine HAVE_UNISTD_H | ||
|
||
/* Define to 1 if you have the `Used' function. */ | ||
#cmakedefine HAVE_USED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, where did you copy to form this config.h.in from? It appears to have unnecessary macros defined such as "Used" and "We", which I think are a result of a bug that I have fixed in c42de06. Can you make this config.h.in more up to date then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's copied from the flex config.h.in. There are quite a bit of entries that are not used. I was being thorough for the sake of preservation. I can make this more current.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would help if you take my config_for_build.h in #373 for reference. That was my attempt to analyze what config.h macros are actually used in Flex, versus what are redundant.
CMakeLists.txt
Outdated
@@ -2,6 +2,10 @@ cmake_minimum_required(VERSION 3.0.2 FATAL_ERROR) | |||
|
|||
set(CMAKE_MODULE_PATH "${CMAKE_MODULE_PATH}" "${CMAKE_SOURCE_DIR}/cmake") | |||
|
|||
if(NOT CMAKE_BUILD_TYPE) | |||
set(CMAKE_BUILD_TYPE Relase) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Thank you
cmake/libfl.pc.in
Outdated
Description: Flex (the fast lexical analyzer) support library | ||
Version: @PACKAGE_VERSION@ | ||
Libs: -L${libdir} -lfl | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excuse me, but why do we need another libfl.pc.in in the flex package? Was it impossible to adopt the src/libfl.pc.in directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Removing it
cmake/flexdef.h
Outdated
char *tmp = strstr(path, fname); | ||
tmp[strlen(fname)] = 0; | ||
return tmp; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The basename() dependency has been removed since my commit fb731ac. Even if it were used, having the function embedded in a header would be bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Again this is a temp file that is going away.
cmake/flexdef.h
Outdated
#endif | ||
#ifdef UINT16_MAX | ||
#undef UINT16_MAX | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This header only applies to ongoing Windows build work, which doesn't work yet. I will remove this until all items are sorted, after which create separate PR.
cmake/flexdef.h
Outdated
|
||
#ifdef HAVE_CONFIG_H | ||
#include <config.h> | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Principally config.h
should be included before other standard headers, as config.h
would contain feature macros that would alter which function interface that standard headers would expose. Is there any reason you include the other way around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes see above comment. I appreciate the time you are putting into reviewing this :)
@jwinarske FYI - Maybe something would be useful. |
What is the status of this? |
What is the status of this?
it's got a low priority. The current focus is automating the release process, getting out 2.6.5 and then integrating bug fixes.
|
How does this cmake script intend to break the dependency circle? |
I can add a bootstrap path. If flex was not found via pkg config, it would
build it.
…On Wed, Apr 1, 2020, 10:41 AM Anonymous Maarten ***@***.***> wrote:
How does this cmake script intend to break the dependency circle?
The cmake script depends on flex itself.
It should build a stage1flex for the build system that can be used to
build flex for the host system.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#378 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC5R3W564N7W3STXX6ELPUTRKN4FLANCNFSM4FS336SQ>
.
|
That would be great! |
Is this still planning on getting merged? CMake generally makes it much easier to build on Windows when it comes to projects that use autotools |
Hey @jwinarske and @westes -- what would be needed to help move this along? I noticed there are a few things that seem in various states of being stalled:
I'm open to taking those PRs across the finish line or helping make requested changes depending on whether or not the original authors are still around and want to keep making changes. If the main issue is the flex maintainer(s) lacking time at the moment, I'm not really sure what could be done -- maybe @westes could chime in here on what would help the most. I have seen a few other projects recently move under an org when the original maintainer was no longer able to keep working on them; I'm guessing that's not the desired approach, but if it is an issue could be opened to discuss further (it seems like there are at least 2 people who have made a number of contributions and have left reviews on PRs). |
There are two sets of issues: things to do with cmake itself and things blocking flex. From what I understand, cmake lacks a number of features that the flex build system expects and has. (If I'm wrong on this, please point me in the right direction.) Autotools works and is standard; cmake may or may not work or whatever, but I've not seen a compelling reason to switch -- beyond that some people are excited about cmake. Maintaining two build systems is a mainightmare over time -- I spent a good deal of time removing all the special purpose stuff thrown in because some systems couldn't be bothered to run the autotools stuff. Yes, a pr that -just- uses github actions, removing travis, would help. While that's maintaining the dependency on github, travis is showing its age and they're clearly moving away from the free support of open source projects. Automating the flex release process is something I need to get back to; that makes releases changes easier. Although, I have to watch that I don't become dependent on github any more than we are. I have considered migrating the flex codebase to an organization setup; the real dependency there is again, I don't want to entangle myself or flex into github than we already are. |
Hope you don't mind me chiming in.
What features?
Since you are the maintainer, this makes sense. I have to assume you're usually running a Linux distribution and developing software from the command line (at least, I do the same). However, some people develop software on Windows. Or, some people deploy software on Windows. When this is the case, having an easy/platform-agnostic way to get Flex is extremely appreciated. With CMake (if Flex used CMake), this could be done on any platform: include(FetchContent)
FetchContent_Declare(flex URL "https://github.com/westes/flex/archive/master.zip")
FetchContent_MakeAvailable(flex)
# Now Flex is available to use in other people's build system. |
fetching the tip of the default branch is going to result in a failed build for most people. That is, therefore, not something I want to make easier. Flex needs to be bootstrapped, one way or another -- either you already have a flex that can build the tip of the default branch or you fetch one of the releases because the released archives include the intermediate files that appear during the bootstrapping process. The most recent lacking feature that I recall was support for getext. My recollection, though, in general, was that while cmake has some interesting features, if you want anything beyond what it offers, you're out of luck. And flex does want some unusual build-time features. With autotools, I've got the shel and standard make to fal back on if I need to make something unusal hapen. But to be clear, the burden of proof is on cmake. We have a working build system. It will run on Windows -- and has for years. Yes it's kind of intricate and hard to wrangle at times. So is bootstrapping flex. |
Or latest release. Just a matter of changing the URL.
I haven't used |
I guess I'm not being clear: What benefit -to flex- does cmake offer? |
Sorry! Not trying to ruffle any feathers here. The benefit is that is makes it easier for people developing for Windows. It benefits users, in that regard. From a maintenance perspective, it may make things more difficult if you're not a regular CMake user. So the way I see it, there's a maintainability / usability trade-off involved with the decision. While I would be happy to see Flex adopt CMake, I think it may be worth supporting CMake in a separate repository. That way, the owner of the repository is responsible for maintaining the build system. |
@tay10r Flex is not going to drop Autotools support since Flex has been part of many GNU/Linux toolchains. The main problem preventing Flex from supporting CMake is: (a) we didn't see much benefit for CMake comparing to maintenance costs, and (b) we don't have people who are experts at CMake so as to keep the Autotools scripts and CMake syntaxes consistent. |
I'm suggesting someone support CMake in a separate repository. That way, the owner of that repository is responsible for keeping the CMake script up to date. It eliminates the burden from primary Flex repository and achieves the same result. |
Alright, that sounds like something I could get a PR prepared for relatively soon.
Any hosting platform will have its own way of managing things that needs to be dealt, particularly if more than one person has write access; self-hosting would come with its own (fun?) management issues. I'll put some thought into ways the core of the release automation could be kept easy to move away from GitHub, for discussion on an automation related issue, so the rest of this can be kept focused on CMake. A few CMake things that come to mind:
I'm still trying to grok exactly what the build system is doing for the tests, though at least with my current understanding I could see a CMake version of the tests being more readable. That said -- it seems like the main obstacle is needing at least one person to take ownership of the CMake side of things to keep it up to date and in-sync with changes to the Autotools build system. Using a separate repository seems like a good way to ensure that it would have a maintainer taking on that responsibility. |
That seems like it would work out well; people who want this feature would have it and it would leave flex maintenance where it is now. |
For the tests, we have the problem that we're testing flex itself. We're testing the flex that we just built, on top of that. We need to test command line options in some cases and we need to test specific things in the flex input in some cases. Since those things break just about every (otherwise reasonable) assumption that automake has about flex, we end up convincing it by changing a lot of default settings that automake has. In some cases, we have to explain rather patiently to automake that the normal ways of building things need to be replaced by various variants. |
One of the biggest problems I have with this kind of request is that flex is part of the bootstrap tooling for many systems. We have to preserve that or else we alienate a lot of the flex userbase. If there were an alternate build system that provided ease of build on other platforms that didn't compromise the other things we need out of flex's build system, I could be pursuaded. While looking around for other things, I found a tool called muon. I see that it has a dependency that states that msvc builds are not supported. Would that make using muon counter to the whole point of using an additional/alternate build system? I have no idea how prevalent various nonstandard, noncompliant compilers are, other than anecdata. (As a side note, my sweet spot on this is having something that autogenerates its own not-Makefiles from Makefile.am and the like. I'd be willing to configure that in the flex codebase and ship autobuilt non-Makefiles with the flex distribution in that case. I also want a tiger made of lightning and a pony with wings.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate of #
After a lot of discussion and evaluation, cmake does not meet flex's needs. At best, this would be a parallel build system that requires constant work to keep up with flex has to do. |
I've done some preliminary work on CMake support.
It builds on Mac, Linux, and partially (6 undefined errors) for Windows.
If this is of any interest, I'm open to feature requests. I was thinking Test cases next.