Thread: Re: fix: propagate M4 env variable to flex subprocess
Hi, On 2025-05-12 23:14:59 -0400, J. Javier Maestro wrote: > The pgflex wrapper runs flex with an explicit environment, so it doesn't > inherit environment variables from the parent process. However, flex can > use the M4 env variable and/or the PATH (via execvp) to find the m4 macro > processor. > Thus, since flex honors the M4 env variable, it should be propagated to the > subprocess environment if it's set in the parent environment. This patch > fixes it. I think it probably was not intentional to fully specify the environment, rather than just *adding* FLEX_TMP_DIR to the caller's environment. Bilal, I think you wrote this originally, do you recall? It seems like an issue beyond just M4... Greetings, Andres Freund
On Tue, May 20, 2025 at 8:53 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
Hi,
On Tue, 13 May 2025 at 18:54, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2025-05-12 23:14:59 -0400, J. Javier Maestro wrote:
> > The pgflex wrapper runs flex with an explicit environment, so it doesn't
> > inherit environment variables from the parent process. However, flex can
> > use the M4 env variable and/or the PATH (via execvp) to find the m4 macro
> > processor.
>
> > Thus, since flex honors the M4 env variable, it should be propagated to the
> > subprocess environment if it's set in the parent environment. This patch
> > fixes it.
>
> I think it probably was not intentional to fully specify the environment,
> rather than just *adding* FLEX_TMP_DIR to the caller's environment. Bilal, I
> think you wrote this originally, do you recall?
I found the original pull request [1] but it does not include the
'FLEX_TMP_DIR' part. I tried to search where the 'FLEX_TMP_DIR' part
is added [2], it seems that this part is added while rebasing so there
is no specific commit.
[1] https://github.com/anarazel/postgres/pull/51
[2] https://github.com/anarazel/postgres/commit/cd817afd4ab006b90307a940d96b5116d649165c
Thanks for the context.
Apart from these pointers to the original PRs, could you (and Andres) give me feedback on the patch? Do you think it's OK to merge? Should I add it to a commitfest?
On this note, on top of that patch, I also have other patches that I think would be relevant. Given that you and Andres seem to use Github, here are the patches that I would like to discuss:
The last one is more of a "hack" but still, it shows the issues with the current approach to "embedding metadata" in header files that end up in the binaries.
What would be the most effective way to discuss these patches? Would it be better to go one-by-one or to create a new thread focused on discussing reproducibility, to discuss all of them?
Thanks a lot beforehand for your help,
Regards,
Javier
Hi, On 2025-05-17 23:32:24 -0400, J. Javier Maestro wrote: > On Tue, May 13, 2025 at 11:54 AM Andres Freund <andres@anarazel.de> wrote: > > Bilal, I think you wrote this originally, do you recall? > > > > It seems like an issue beyond just M4... > > > > IIRC the rest of the tools in the environment have ways to be specified via > Meson options (BISON, FLEX, PERL) so the only issue I see is Flex not being > able to find the specific m4 binary. What other issue(s) are you > considering? PATH, LD_LIBRARY_PATH, ... I think this really should just add to the environment, rather than supplant it. Do you want to write a patch like that? Otherwise I can. Greetings, Andres Freund
On Wed, May 28, 2025 at 6:08 PM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2025-05-17 23:32:24 -0400, J. Javier Maestro wrote:
> On Tue, May 13, 2025 at 11:54 AM Andres Freund <andres@anarazel.de> wrote:
> > Bilal, I think you wrote this originally, do you recall?
> >
> > It seems like an issue beyond just M4...
> >
>
> IIRC the rest of the tools in the environment have ways to be specified via
> Meson options (BISON, FLEX, PERL) so the only issue I see is Flex not being
> able to find the specific m4 binary. What other issue(s) are you
> considering?
PATH, LD_LIBRARY_PATH, ...
I think this really should just add to the environment, rather than supplant
it.
Ah, understood. That definitely looks like a better option.
Do you want to write a patch like that? Otherwise I can.
Sure, I've attached the new patch. Let me know what you think, and if it's OK, what are the next steps to get the patch merged in main!
Thanks,
Javier