Обсуждение: 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 13, 2025 at 11:54 AM 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.
I think so, it definitely looks like the only intent was to specify FLEX_TMP_DIR.
But even if the goal wasn’t to fully specify the environment, this fix is only passing an env variable that's supposed to be there because Flex will honor it if set.
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?
Thanks!
Javier
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 -- Regards, Nazir Bilal Yavuz Microsoft
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
Вложения
On 28.05.25 20:42, J. Javier Maestro wrote: > On Wed, May 28, 2025 at 6:08 PM Andres Freund <andres@anarazel.de > <mailto: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 <mailto: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! This patch looks right to me. I would wait for the PG19 branching at this point, unless there is a concrete need for backpatching.
On Wed, May 28, 2025 at 11:43 AM J. Javier Maestro <jjmaestro@ieee.org> wrote:
On Wed, May 28, 2025 at 6:08 PM Andres Freund <andres@anarazel.de> wrote:
...
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!
I checked 0001 version, it builds and works as expected.
Not to lose it, created commitfest entry: https://commitfest.postgresql.org/patch/5835/
(and marked as ready, considering others' words in this thread)
On Tue, Jun 17, 2025 at 6:15 AM Peter Eisentraut <peter@eisentraut.org> wrote:
This patch looks right to me.
Great, thanks!
I would wait for the PG19 branching at this point, unless there is a
concrete need for backpatching.
This patch fixes just one of the issues I found when trying to have a reproducible PG build... and, given the fact that there are still other issues that make a PG build non-reproducible (see the other patches in https://github.com/monogres/postgres/compare/REL_16_0...monogres/patches/16.0) I'd say it's OK if it's not backported.
Thank you!
Javier
On Sun, Jun 22, 2025 at 10:23 PM Nikolay Samokhvalov <nik@postgres.ai> wrote:
On Wed, May 28, 2025 at 11:43 AM J. Javier Maestro <jjmaestro@ieee.org> wrote:On Wed, May 28, 2025 at 6:08 PM Andres Freund <andres@anarazel.de> wrote:...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!I checked 0001 version, it builds and works as expected.Not to lose it, created commitfest entry: https://commitfest.postgresql.org/patch/5835/(and marked as ready, considering others' words in this thread)
Thank you!
Cheers,
Javier
On 17.06.25 07:15, Peter Eisentraut wrote: > On 28.05.25 20:42, J. Javier Maestro wrote: >> On Wed, May 28, 2025 at 6:08 PM Andres Freund <andres@anarazel.de >> <mailto: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 <mailto: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! > > This patch looks right to me. > > I would wait for the PG19 branching at this point, unless there is a > concrete need for backpatching. committed
On Mon, Jun 30, 2025 at 11:25 AM Peter Eisentraut <peter@eisentraut.org> wrote:
On 17.06.25 07:15, Peter Eisentraut wrote:
> On 28.05.25 20:42, J. Javier Maestro wrote:
>> On Wed, May 28, 2025 at 6:08 PM Andres Freund <andres@anarazel.de
>> <mailto: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 <mailto: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!
>
> This patch looks right to me.
>
> I would wait for the PG19 branching at this point, unless there is a
> concrete need for backpatching.
committed
Thanks!
Javier