[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Communicating about the change in behaviour for ar



Manoj Srivastava:
>         I was trying to import the new version of make into unstable, and
>  I discovered that t/10 tests about the archive related part of makes
>  test suite failed. The reason was a change in the behaviour of ar,
>  which is now configured with --enable-deterministic-archives. When
>  adding files and the archive index use zero for UIDs, GIDs, timestamps,
>  and use consistent file modes for all files.  When this option is used,
>  if ar is used with identical options and identical input files,
>  multiple runs will create identical output files regardless of the
>  input files' owners, groups, file modes, or modification times. This
>  seems like good news for reproducible builds.
> 
>         Unfortunately, when using makes libxx(*.a) syntax for creating
>  archives, make needs the timestamp of the file in order to decide to
>  update it or not. With the current deterministic behavior of ar, the
>  timestamp is always 0. This is behaviour that is not backwards
>  compatible (as can be seen in the bug report #798804 and
>  #798913). Some operations, instead of being no-ops, now rebuild theg/Lunar
>  archive.
> 
>         T think the change, because of the benefits of the
>  reproducible builds, are a good thing, but I also think that they are
>  not visible to the general user base (not all the users of ar and make
>  are debian developers, and the release is not the only thing built
>  using ar and make).  My recommendations is a NEWS file entry for
>  binutils and make; and a mention in the release announcement for
>  reproducible builds.
> 
>         I have uploaded a version of make the defaults to adding U to the
>  ARFLAGS, but, on research and reflection, I would be open to reverting
>  that and adding a NEWS entry.

In any case, I think we should communicate to users that something
unexpected (from the point view of make) is happening so they can adapt
their Makefile. What would you think of the attached patch series?

The warning is actually implemented in the second patch, but the first
one is required to be able to differentiate a non-existent archive or
member from a member with a timestamp set to 0.

-- 
Lunar                                .''`. 
lunar@debian.org                    : :Ⓐ  :  # apt-get install anarchism
                                    `. `'` 
                                      `-   
From a40d198fb5a3a0387ce5b4f0f40e23cab2a535bb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Bobbio?= <lunar@debian.org>
Date: Sun, 17 Jan 2016 10:55:40 +0000
Subject: [PATCH 1/2] Make ar_member_date compatible with archives with
 timestamps set to 0

ar_scan() scanning function uses 0 to indicate that scanning should continue.
This made ar_member_date() unable to differentiate when it was unable to find
the requested member from a member with a timestamp set to 0.

We thus change its prototype to have its return value indicate if it has been
able to find the requested member. The timestamp is set through the newly given
pointer.
---
 ar.c       | 33 +++++++++++++++++++++++++--------
 commands.c |  5 ++++-
 dir.c      |  7 +++++--
 makeint.h  |  2 +-
 remake.c   |  7 +++----
 5 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/ar.c b/ar.c
index 675572a..d10a8df 100644
--- a/ar.c
+++ b/ar.c
@@ -68,25 +68,39 @@ ar_parse_name (const char *name, char **arname_p, char **memname_p)
 
 /* This function is called by 'ar_scan' to find which member to look at.  */
 
+struct member_date_lookup
+{
+  const char *name;
+  time_t *member_date;
+};
+
 /* ARGSUSED */
 static long int
 ar_member_date_1 (int desc UNUSED, const char *mem, int truncated,
                   long int hdrpos UNUSED, long int datapos UNUSED,
                   long int size UNUSED, long int date,
                   int uid UNUSED, int gid UNUSED, int mode UNUSED,
-                  const void *name)
+                  const void *data)
 {
-  return ar_name_equal (name, mem, truncated) ? date : 0;
+  const struct member_date_lookup *lookup_data = data;
+  if (ar_name_equal (lookup_data->name, mem, truncated))
+    {
+      *lookup_data->member_date = date;
+      return 1;
+    }
+  return 0;
 }
 
-/* Return the modtime of NAME.  */
+/* Read the modtime of NAME in MEMBER_DATE.
+   Returns 1 if NAME exists, 0 otherwise.  */
 
-time_t
-ar_member_date (const char *name)
+int
+ar_member_date (const char *name, time_t *member_date)
 {
   char *arname;
   char *memname;
-  long int val;
+  int found;
+  struct member_date_lookup lookup_data;
 
   ar_parse_name (name, &arname, &memname);
 
@@ -107,11 +121,14 @@ ar_member_date (const char *name)
       (void) f_mtime (arfile, 0);
   }
 
-  val = ar_scan (arname, ar_member_date_1, memname);
+  lookup_data.name = memname;
+  lookup_data.member_date = member_date;
+  found = ar_scan (arname, ar_member_date_1, &lookup_data);
 
   free (arname);
 
-  return (val <= 0 ? (time_t) -1 : (time_t) val);
+  /* return 0 (not found) if the archive does not exist or has invalid format. */
+  return (found == 1) ? 1 : 0;
 }
 
 /* Set the archive-member NAME's modtime to now.  */
diff --git a/commands.c b/commands.c
index 7123021..029f18e 100644
--- a/commands.c
+++ b/commands.c
@@ -622,7 +622,10 @@ delete_target (struct file *file, const char *on_behalf_of)
       time_t file_date = (file->last_mtime == NONEXISTENT_MTIME
                           ? (time_t) -1
                           : (time_t) FILE_TIMESTAMP_S (file->last_mtime));
-      if (ar_member_date (file->name) != file_date)
+      time_t member_date = NONEXISTENT_MTIME;
+      int found;
+      found = ar_member_date (file->name, &member_date);
+      if (found && member_date != file_date)
         {
           if (on_behalf_of)
             OSS (error, NILF,
diff --git a/dir.c b/dir.c
index 7e00b8f..6b8881f 100644
--- a/dir.c
+++ b/dir.c
@@ -748,8 +748,11 @@ file_exists_p (const char *name)
   const char *slash;
 
 #ifndef NO_ARCHIVES
-  if (ar_name (name))
-    return ar_member_date (name) != (time_t) -1;
+  {
+    time_t member_date;
+    if (ar_name (name))
+      return ar_member_date (name, &member_date);
+  }
 #endif
 
 #ifdef VMS
diff --git a/makeint.h b/makeint.h
index fdcae75..a236884 100644
--- a/makeint.h
+++ b/makeint.h
@@ -478,7 +478,7 @@ const char *find_percent_cached (const char **);
 int ar_name (const char *);
 void ar_parse_name (const char *, char **, char **);
 int ar_touch (const char *);
-time_t ar_member_date (const char *);
+int ar_member_date (const char *, time_t *);
 
 typedef long int (*ar_member_func_t) (int desc, const char *mem, int truncated,
                                       long int hdrpos, long int datapos,
diff --git a/remake.c b/remake.c
index dc9d41f..24da0d7 100644
--- a/remake.c
+++ b/remake.c
@@ -1259,6 +1259,7 @@ f_mtime (struct file *file, int search)
 
       char *arname, *memname;
       struct file *arfile;
+      int found;
       time_t member_date;
 
       /* Find the archive's name.  */
@@ -1306,10 +1307,8 @@ f_mtime (struct file *file, int search)
         /* The archive doesn't exist, so its members don't exist either.  */
         return NONEXISTENT_MTIME;
 
-      member_date = ar_member_date (file->hname);
-      mtime = (member_date == (time_t) -1
-               ? NONEXISTENT_MTIME
-               : file_timestamp_cons (file->hname, member_date, 0));
+      found = ar_member_date (file->hname, &member_date);
+      mtime = found ? file_timestamp_cons (file->hname, member_date, 0) : NONEXISTENT_MTIME;
     }
   else
 #endif
-- 
2.6.1

From 0f83fc31e81fca5e7022437869b72ad2a07051e7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Bobbio?= <lunar@debian.org>
Date: Sun, 17 Jan 2016 11:01:09 +0000
Subject: [PATCH 2/2] Issue a warning when we detect a "deterministic" archive

binutils will create archive with timestamps set to 0 when running in
"deterministic" mode. As make will always try to update such members,
let's notify users with a warning.
---
 remake.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/remake.c b/remake.c
index 24da0d7..123a260 100644
--- a/remake.c
+++ b/remake.c
@@ -1308,6 +1308,13 @@ f_mtime (struct file *file, int search)
         return NONEXISTENT_MTIME;
 
       found = ar_member_date (file->hname, &member_date);
+      if (found && member_date == (time_t) 0)
+        {
+              OSS (error, NILF,
+                   _("Warning: Archive '%s' seems to have been created in deterministic mode. '%s' will always be updated. Please consider passing the U flag to ar to avoid the problem."),
+                   arfile->name, memname);
+
+        }
       mtime = found ? file_timestamp_cons (file->hname, member_date, 0) : NONEXISTENT_MTIME;
     }
   else
-- 
2.6.1

Attachment: signature.asc
Description: Digital signature


Reply to: