Remove specific \r\n code in TAP for Windows

Started by Michael Paquier3 months ago6 messages
#1Michael Paquier
michael@paquier.xyz
1 attachment(s)

Hi all,

$subject has been mentioned by Jacob (in CC.) on Discord, and it does
not seem like somebody took the time to propose a patch.

We use in some places of the TAP tests the following pattern:
my $newline = $windows_os ? "\r\n" : "\n";

Two files have this idea:
- 003_ldap_connection_param_lookup.pl
- 006_service.pl
Note: I have committed one of these.

However, in light of 1c6d4629394d, we should not require that, and the
CI looks happy with the attached.

That would be up to the buildfarm to act as final judge, but any
objections in attempting to get rid of these like in the attached?
I would try first HEAD to be sure, then follow with an optional
backpatch.

Thoughts?
--
Michael

Attachments:

0001-Remove-specific-CRLF-code-in-TAP-tests.patchtext/x-diff; charset=us-asciiDownload
From 150097988b42b3c26aec2f99c99f6fbe76ca8f8b Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Fri, 24 Oct 2025 15:23:59 +0900
Subject: [PATCH] Remove specific CRLF code in TAP tests

---
 src/interfaces/libpq/t/006_service.pl         | 12 +++----
 .../t/003_ldap_connection_param_lookup.pl     | 32 ++++++-------------
 2 files changed, 13 insertions(+), 31 deletions(-)

diff --git a/src/interfaces/libpq/t/006_service.pl b/src/interfaces/libpq/t/006_service.pl
index 797e6232b8fc..29a70629b448 100644
--- a/src/interfaces/libpq/t/006_service.pl
+++ b/src/interfaces/libpq/t/006_service.pl
@@ -22,18 +22,14 @@ $dummy_node->init;
 
 my $td = PostgreSQL::Test::Utils::tempdir;
 
-# Windows vs non-Windows: CRLF vs LF for the file's newline, relying on
-# the fact that libpq uses fgets() when reading the lines of a service file.
-my $newline = $windows_os ? "\r\n" : "\n";
-
 # Create the set of service files used in the tests.
 # File that includes a valid service name, and uses a decomposed connection
 # string for its contents, split on spaces.
 my $srvfile_valid = "$td/pg_service_valid.conf";
-append_to_file($srvfile_valid, "[my_srv]" . $newline);
+append_to_file($srvfile_valid, "[my_srv]\n");
 foreach my $param (split(/\s+/, $node->connstr))
 {
-	append_to_file($srvfile_valid, $param . $newline);
+	append_to_file($srvfile_valid, $param . "\n");
 }
 
 # File defined with no contents, used as default value for PGSERVICEFILE,
@@ -51,14 +47,14 @@ my $srvfile_missing = "$td/pg_service_missing.conf";
 my $srvfile_nested = "$td/pg_service_nested.conf";
 copy($srvfile_valid, $srvfile_nested)
   or die "Could not copy $srvfile_valid to $srvfile_nested: $!";
-append_to_file($srvfile_nested, 'service=invalid_srv' . $newline);
+append_to_file($srvfile_nested, "service=invalid_srv\n");
 
 # Service file with nested "servicefile" defined.
 my $srvfile_nested_2 = "$td/pg_service_nested_2.conf";
 copy($srvfile_valid, $srvfile_nested_2)
   or die "Could not copy $srvfile_valid to $srvfile_nested_2: $!";
 append_to_file($srvfile_nested_2,
-	'servicefile=' . $srvfile_default . $newline);
+	'servicefile=' . $srvfile_default . "\n");
 
 # Set the fallback directory lookup of the service file to the temporary
 # directory of this test.  PGSYSCONFDIR is used if the service file
diff --git a/src/test/ldap/t/003_ldap_connection_param_lookup.pl b/src/test/ldap/t/003_ldap_connection_param_lookup.pl
index 8c1e1caf992a..a7f2bdf23ee9 100644
--- a/src/test/ldap/t/003_ldap_connection_param_lookup.pl
+++ b/src/test/ldap/t/003_ldap_connection_param_lookup.pl
@@ -44,31 +44,18 @@ $ldap->ldapadd_file('authdata.ldif');
 $ldap->ldapsetpw('uid=test1,dc=example,dc=net', 'secret1');
 $ldap->ldapsetpw('uid=test2,dc=example,dc=net', 'secret2');
 
-# Windows vs non-Windows: CRLF vs LF for the file's newline, relying on
-# the fact that libpq uses fgets() when reading the lines of a service file.
-my $newline = $windows_os ? "\r\n" : "\n";
-
 my $td = PostgreSQL::Test::Utils::tempdir;
 
 # create ldap file based on postgres connection info
 my $ldif_valid = "$td/connection_params.ldif";
-append_to_file($ldif_valid, "version:1");
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "dn:cn=mydatabase,dc=example,dc=net");
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "changetype:add");
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "objectclass:top");
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "objectclass:device");
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "cn:mydatabase");
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "description:host=");
-append_to_file($ldif_valid, $node->host);
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "description:port=");
-append_to_file($ldif_valid, $node->port);
+append_to_file($ldif_valid, "version:1\n");
+append_to_file($ldif_valid, "dn:cn=mydatabase,dc=example,dc=net\n");
+append_to_file($ldif_valid, "changetype:add\n");
+append_to_file($ldif_valid, "objectclass:top\n");
+append_to_file($ldif_valid, "objectclass:device\n");
+append_to_file($ldif_valid, "cn:mydatabase\n");
+append_to_file($ldif_valid, "description:host=" . $node->host . "\n");
+append_to_file($ldif_valid, "description:port=" . $node->port);
 
 $ldap->ldapadd_file($ldif_valid);
 
@@ -86,8 +73,7 @@ note "setting up PostgreSQL instance";
 # File that includes a valid service name, that uses a decomposed
 # connection string for its contents, split on spaces.
 my $srvfile_valid = "$td/pg_service_valid.conf";
-append_to_file($srvfile_valid, "[my_srv]");
-append_to_file($srvfile_valid, $newline);
+append_to_file($srvfile_valid, "[my_srv]\n");
 append_to_file($srvfile_valid, "ldap://localhost:");
 append_to_file($srvfile_valid, $ldap_port);
 append_to_file($srvfile_valid,
-- 
2.51.0

#2Daniel Gustafsson
daniel@yesql.se
In reply to: Michael Paquier (#1)
Re: Remove specific \r\n code in TAP for Windows

On 24 Oct 2025, at 08:51, Michael Paquier <michael@paquier.xyz> wrote:

in light of 1c6d4629394d, we should not require that, and the
CI looks happy with the attached.

+1, a nice win in terms of readability.

--
Daniel Gustafsson

#3Jacob Champion
jacob.champion@enterprisedb.com
In reply to: Michael Paquier (#1)
1 attachment(s)
Re: Remove specific \r\n code in TAP for Windows

On Thu, Oct 23, 2025 at 11:51 PM Michael Paquier <michael@paquier.xyz> wrote:

$subject has been mentioned by Jacob (in CC.) on Discord, and it does
not seem like somebody took the time to propose a patch.

Ah, sorry, I put part of my first attempt at it over in [1]/messages/by-id/CAOYmi+kYKPXCfiAF3xAu+cHYSLYEc4OC9Wsh2nebwWboNbkpeQ@mail.gmail.com. But I
realize that's not very visible, nor does it address
003_ldap_connection_param_lookup.

I'm carrying a local diff that switches 003_ldap_ over to qq{} style.
Attached in case you like it, but your patch LGTM too.

That would be up to the buildfarm to act as final judge, but any
objections in attempting to get rid of these like in the attached?
I would try first HEAD to be sure, then follow with an optional
backpatch.

Sounds like a plan.

Thanks!
--Jacob

[1]: /messages/by-id/CAOYmi+kYKPXCfiAF3xAu+cHYSLYEc4OC9Wsh2nebwWboNbkpeQ@mail.gmail.com

Attachments:

003_ldap_newlines.diff.txttext/plain; charset=US-ASCII; name=003_ldap_newlines.diff.txtDownload
commit aba4fc95f1a68674ce5c7c2c57e59073708bcfe3
Author: Jacob Champion <jacob.champion@enterprisedb.com>
Date:   Mon Oct 13 11:47:08 2025

    WIP: simplify LDAP test newlines too

diff --git a/src/test/ldap/t/003_ldap_connection_param_lookup.pl b/src/test/ldap/t/003_ldap_connection_param_lookup.pl
index 8c1e1caf992..abcb6d22a8b 100644
--- a/src/test/ldap/t/003_ldap_connection_param_lookup.pl
+++ b/src/test/ldap/t/003_ldap_connection_param_lookup.pl
@@ -44,31 +44,21 @@ $ldap->ldapadd_file('authdata.ldif');
 $ldap->ldapsetpw('uid=test1,dc=example,dc=net', 'secret1');
 $ldap->ldapsetpw('uid=test2,dc=example,dc=net', 'secret2');
 
-# Windows vs non-Windows: CRLF vs LF for the file's newline, relying on
-# the fact that libpq uses fgets() when reading the lines of a service file.
-my $newline = $windows_os ? "\r\n" : "\n";
-
 my $td = PostgreSQL::Test::Utils::tempdir;
 
 # create ldap file based on postgres connection info
 my $ldif_valid = "$td/connection_params.ldif";
-append_to_file($ldif_valid, "version:1");
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "dn:cn=mydatabase,dc=example,dc=net");
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "changetype:add");
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "objectclass:top");
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "objectclass:device");
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "cn:mydatabase");
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "description:host=");
-append_to_file($ldif_valid, $node->host);
-append_to_file($ldif_valid, $newline);
-append_to_file($ldif_valid, "description:port=");
-append_to_file($ldif_valid, $node->port);
+append_to_file(
+	$ldif_valid, qq{
+version:1
+dn:cn=mydatabase,dc=example,dc=net
+changetype:add
+objectclass:top
+objectclass:device
+cn:mydatabase
+description:host=} . $node->host . qq{
+description:port=} . $node->port . qq{
+});
 
 $ldap->ldapadd_file($ldif_valid);
 
@@ -86,12 +76,11 @@ note "setting up PostgreSQL instance";
 # File that includes a valid service name, that uses a decomposed
 # connection string for its contents, split on spaces.
 my $srvfile_valid = "$td/pg_service_valid.conf";
-append_to_file($srvfile_valid, "[my_srv]");
-append_to_file($srvfile_valid, $newline);
-append_to_file($srvfile_valid, "ldap://localhost:");
-append_to_file($srvfile_valid, $ldap_port);
-append_to_file($srvfile_valid,
-	"/dc=example,dc=net?description?one?(cn=mydatabase)");
+append_to_file(
+	$srvfile_valid, qq{
+[my_srv]
+ldap://localhost:$ldap_port/dc=example,dc=net?description?one?(cn=mydatabase)
+});
 
 # File defined with no contents, used as default value for
 # PGSERVICEFILE, so that no lookup is attempted in the user's home
#4Michael Paquier
michael@paquier.xyz
In reply to: Jacob Champion (#3)
Re: Remove specific \r\n code in TAP for Windows

On Mon, Oct 27, 2025 at 08:33:57AM -0700, Jacob Champion wrote:

Ah, sorry, I put part of my first attempt at it over in [1]. But I
realize that's not very visible, nor does it address
003_ldap_connection_param_lookup.

Ahh, so you did post something. I have missed that

I'm carrying a local diff that switches 003_ldap_ over to qq{} style.
Attached in case you like it, but your patch LGTM too.

Your suggestions for the LDAP test are cleaner than what I came up
with, so reused these.

That would be up to the buildfarm to act as final judge, but any
objections in attempting to get rid of these like in the attached?
I would try first HEAD to be sure, then follow with an optional
backpatch.

Sounds like a plan.

Time for a verdict, so applied on HEAD now.
--
Michael

#5Michael Paquier
michael@paquier.xyz
In reply to: Michael Paquier (#4)
Re: Remove specific \r\n code in TAP for Windows

On Tue, Oct 28, 2025 at 08:27:35AM +0900, Michael Paquier wrote:

Time for a verdict, so applied on HEAD now.

Three buildfarm members have reported after 8767b449a3a1: fairywren,
drongo and hamerkop. All of them have reported green, so applied the
equivalent cleanup on REL_18_STABLE with a92bbffbc3a7 for the service
test.
--
Michael

#6Jacob Champion
jacob.champion@enterprisedb.com
In reply to: Michael Paquier (#5)
Re: Remove specific \r\n code in TAP for Windows

On Tue, Oct 28, 2025 at 6:08 PM Michael Paquier <michael@paquier.xyz> wrote:

Three buildfarm members have reported after 8767b449a3a1: fairywren,
drongo and hamerkop. All of them have reported green, so applied the
equivalent cleanup on REL_18_STABLE with a92bbffbc3a7 for the service
test.

Thank you!

--Jacob