feat: backport flex noline fix

[jjmaestro]

When using the --noline flag or the %option noline, flex was failing to suppress the %top line directives.

There was an if-guard missing in the code that adds the line directives to the top_buf. I've fixed it in westes/flex#704.

This commit adds patching capabilities to ease maintaining fixes / backports like the noline fix.

P.S. As @Smattr mentioned in #268 this fix is important for reproducibility, in Debian and many other projects, like e.g. in a project I'm working on (making a reproducible Postgres, amongst other things... that's how I found out about the bug :) )

[jmillikin]

I'm not opposed to the idea, but have some questions/concerns about the details.

First, to be a backport the original change would need to have been accepted upstream. The diff in your PR westes/flex#704 looks good to me, but it's not been merged into Flex yet, so it's more of a Debian-style ad-hoc patch.

Second, I'm hesitant to apply patches that change the output of Flex. If a user configures their build with flex.repository(name = "flex", version = "2.6.4") then they expect the behavior of Flex v2.6.4, not flex-v2.6.4+patches-jmillikin-thought-were-good-ideas-1. This isn't a hard rule, and patches that fix obvious bugs are the easiest to justify, but there does still need to be a good reason for each such patch.

Third, I don't understand the relationship between your patch and reproducibility. The #line directives generated by Flex are based on the input filenames, which when invoked by Bazel are relative paths. Thus, the existing behavior should be deterministic / reproducible already.

Some non-Bazel build systems use absolute paths so they have to use --noline to avoid things like #line "/build-sandbox/abc123randomhash/src/foo.l, but that's not the case in Bazel. Could you provide an example of how removing the #line directive in question fixes a reproducibility issue?

[jmillikin]

Also, in case you do need to completely remove #line pragmas from the output, the rules_flex rules have a flex_options attribute that can be used to adjust the Flex command line options. Setting flex_options = ["--noline"] will suppress #line pragmas regardless of whether %option noline is set, and per the linked issue it suppresses even the initial one.

[jjmaestro]

@jmillikin thanks for your comments, lots of good insights! Let me reply bottom-up, I think it will explain the PR better (I should have added more info and context from the beginning, sorry about that!):

Also, in case you do need to completely remove #line pragmas from the output, the rules_flex rules have a flex_options attribute (...)

flex_options is actually what I wanted to use because, as you said, it will pass the CLI option and will make Flex omit the line directives regardless of using %option noline.

But the problem is in Flex itself, without the patch, the %top section will always print a #line directive because the Flex code is missing an if-guard.

You can check, just removing the patch I added will break the test I wrote in the PR (which has the --noline hardcoded, but you can also try without the CLI argument and using %option noline, it will still break).

First, to be a backport the original change would need to have been accepted upstream. The diff (...) looks good to me, but it's not been merged into Flex yet

Agreed, I moved too fast and thought it was going to be accepted 😅. Luckily, it has just been merged upstream! 🎉

Second, I'm hesitant to apply patches that change the output of Flex. (...) This isn't a hard rule, and patches that fix obvious bugs are the easiest to justify, but there does still need to be a good reason for each such patch.

Yeah, I'm 100% with you on this. The only reason I proposed the PR was because it's a very clear bug, there were reports about this bug from many years ago and similar patches were previously accepted in upstream Flex.

Third, I don't understand the relationship between your patch and reproducibility. The #line directives generated by Flex are based on the input filenames, which when invoked by Bazel are relative paths. (...) Some non-Bazel build systems use absolute paths (...)

My exact case :) I'm building a non-Bazel project (Posgres uses Meson build) with rules_foreign_cc meson rule 😅 Without the --noline, there's a bunch of paths that end up in the binary.

Hopefully this clarifies the PR, let me know what you think and/or if you want any changes!

[jmillikin]

Quick note, sorry -- I've not forgotten about this but haven't had time this week due to other matters.

My rough plan is to add a patches= attribute to flex_repository, have it default to {"2.6.4": ["@rules_flex//patches:0001-fix-noline-for-top-directives.patch"]} or some such, and have all patches in that attribute get applied to the selected version. This should provide the fixed behavior for most users, with advanced use cases (requiring exact behavioral compatibility) being able to manually set patches = {}.

The resulting commit will be somewhat different than what's in this PR, but I plan to preserve the authorship metadata (i.e. set you as the Author:). Please tell me if you'd prefer otherwise.

[jjmaestro]

No worries! I'm now away from a computer until next week :)

Yeah, I did the patching with the idea of having a way to eventually move the changes in e.g. _hardcode_int_max_log10() into patches, I thought it could probably make it easier / clearer if those were in patches instead of in code.

I'll be back to civilization with Internet and a computer on Tuesday, if you want to do it earlier no worries but I'm also happy to change the patch and do your approach. Whatever works!

[jjmaestro]

@jmillikin I've made the changes we discussed. I think this version is better, as you said, it allows people to override the behavior setting patches = {}. Let me know what you think!

[jjmaestro]

@jmillikin ping? :)

[jmillikin]

Sorry, this is in my inbox but non-computer life has been busy and I haven't had time for GitHub things.

The current diff looks good to me; if you need this in a downstream build then feel free to use the current diff as a patch against rules_flex. When I merge this in and cut the next release its API will be the same as the current diff.

[jjmaestro]

@jmillikin cool, no worries! All I wanted to know is if the PR needed more changes :) Thanks!!