Subject: new feature for bmake: read-only variables
To: None <tech-pkg@netbsd.org>
From: Roland Illig <rillig@NetBSD.org>
List: tech-pkg
Date: 08/25/2005 12:51:36
This is a multi-part message in MIME format.
--------------050907040208000803020706
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

This patch adds a new feature to bmake. Variables can be marked 
read-only, so there is no chance of overwriting them.

PKGNAME=	mypackage-1.4
.readonly PKGNAME

This code will work as usual, but as soon as PKGREVISION is defined, it 
will blow up in ../../mk/bsd.pkg.mk, line 86. At that point, pkgsrc is 
modifying the variable. I consider silent variable modification harmful, 
because it makes debugging much harder.

For convenience, there are two more directives, .begin_readonly and 
.end_readonly, which can be used like this:

.begin_readonly
TOOLS_PLATFORM.cat?=            /bin/cat
TOOLS_PLATFORM.chgrp?=          /usr/bin/chgrp
TOOLS_PLATFORM.chmod?=          /bin/chmod
TOOLS_PLATFORM.chown?=          /usr/sbin/chown
TOOLS_PLATFORM.cmp?=            /usr/bin/cmp
TOOLS_PLATFORM.cp?=             /bin/cp
TOOLS_PLATFORM.csh?=            /bin/csh
TOOLS_PLATFORM.cut?=            /usr/bin/cut
TOOLS_PLATFORM.date?=           /bin/date
TOOLS_PLATFORM.diff?=           /usr/bin/diff
TOOLS_PLATFORM.dirname?=        /usr/bin/dirname
TOOLS_PLATFORM.echo?=           echo                    # shell builtin
TOOLS_PLATFORM.egrep?=          /usr/bin/egrep
TOOLS_PLATFORM.env?=            /usr/bin/env
.end_readonly

I don't expect this feature to be integrated immediately, it's just that 
I find it useful for debugging. And, if it would be applied to pkgsrc, 
it would make its behavior more predictable.

By the way, when preparing this patch I have rewritten the directive 
parser, because the old code (from 1993 and 1994) looked too ugly. The 
new parser code is a little stricter, in that the directive .undefined 
does not mean "delete the variable ined" anymore.

Roland

--------------050907040208000803020706
Content-Type: text/plain;
 name="bmake-readonly-vars.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="bmake-readonly-vars.patch"

? bmake-readonly-vars.patch
Index: nonints.h
===================================================================
RCS file: /cvsroot/pkgsrc/bootstrap/bmake/nonints.h,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 nonints.h
--- nonints.h	11 Mar 2004 13:04:10 -0000	1.1.1.1
+++ nonints.h	25 Aug 2005 10:32:37 -0000
@@ -152,3 +152,5 @@ char *Var_GetHead __P((char *));
 void Var_Init __P((void));
 void Var_End __P((void));
 void Var_Dump __P((GNode *));
+void Var_Make_Readonly(char *, GNode *);
+void Var_Set_Readonly_Mode(int);
Index: parse.c
===================================================================
RCS file: /cvsroot/pkgsrc/bootstrap/bmake/parse.c,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 parse.c
--- parse.c	11 Mar 2004 13:04:12 -0000	1.1.1.1
+++ parse.c	25 Aug 2005 10:32:37 -0000
@@ -2534,6 +2534,31 @@ ParseFinishLine()
     }
 }
 
+static char *
+skip_word(char *cp)
+{
+    while (*cp != '\0' && !isspace((unsigned char)*cp))
+	cp++;
+    return cp;
+}
+
+static char *
+skip_whitespace(char *cp)
+{
+    while (isspace((unsigned char)*cp))
+	cp++;
+    return cp;
+}
+
+static int
+word_eq(const char *begin, const char *end, const char *word)
+{
+    size_t word_len;
+
+    word_len = strlen(word);
+    return (end - begin == word_len
+            && memcmp(begin, word, end - begin) == 0);
+}
 
 /*-
  *---------------------------------------------------------------------
@@ -2568,36 +2593,66 @@ Parse_File(name, stream)
 
     do {
 	while ((line = ParseReadLine ()) != NULL) {
+
+	    /* Check if the line is a directive. */
 	    if (*line == '.') {
+		char *dir, *dir_end, *cont;
+
 		/*
-		 * Lines that begin with the special character are either
-		 * include or undef directives.
+		 * Get the name of the directive. The {cont} pointer
+		 * tells us where to continue parsing after the directive
+		 * has processed its arguments.
 		 */
-		for (cp = line + 1; isspace ((unsigned char)*cp); cp++) {
-		    continue;
-		}
-		if (strncmp(cp, "include", 7) == 0 ||
-	    	    ((cp[0] == 's' || cp[0] == '-') &&
-		    strncmp(&cp[1], "include", 7) == 0)) {
-		    ParseDoInclude (cp);
+		dir = skip_whitespace(line + 1);
+		dir_end = skip_word(dir);
+		cont = dir_end;
+
+		if (word_eq(dir, dir_end, "include") ||
+		    word_eq(dir, dir_end, "sinclude") ||
+		    word_eq(dir, dir_end, "-include")) {
+		    ParseDoInclude (dir);
 		    goto nextLine;
-		} else if (strncmp(cp, "undef", 5) == 0) {
-		    char *cp2;
-		    for (cp += 5; isspace((unsigned char) *cp); cp++) {
-			continue;
-		    }
 
-		    for (cp2 = cp; !isspace((unsigned char) *cp2) &&
-				   (*cp2 != '\0'); cp2++) {
-			continue;
-		    }
+		} else if (word_eq(dir, dir_end, "undef")) {
+		    char *varname;
 
-		    *cp2 = '\0';
+		    varname = skip_whitespace(dir_end);
+		    cont = skip_word(varname);
 
-		    Var_Delete(cp, VAR_GLOBAL);
-		    goto nextLine;
+		    Var_Delete(varname, VAR_GLOBAL);
+
+		} else if (word_eq(dir, dir_end, "readonly")) {
+		    char *varname;
+
+		    varname = skip_whitespace(dir_end);
+		    cont = skip_word(varname);
+
+		    Var_Make_Readonly(varname, VAR_GLOBAL);
+
+		} else if (word_eq(dir, dir_end, "begin_readonly")) {
+		    Var_Set_Readonly_Mode(1);
+
+		} else if (word_eq(dir, dir_end, "end_readonly")) {
+		    Var_Set_Readonly_Mode(0);
+
+		} else if (strchr(line, ':') == NULL) {
+		    *dir_end = '\0';
+		    Parse_Error(PARSE_FATAL, "Unknown directive \"%s\".", dir);
+		    /* NOTREACHED */
+		} else {
+		    /* Can be something like .SUFFIXES: or .c.o: */
+		    goto its_not_a_directive /* below */;
 		}
+
+		cont = skip_whitespace(cont);
+		if (*cont == '\0' || *cont == '#')
+		    goto nextLine;
+
+		Parse_Error(PARSE_FATAL, "Garbage at end of line: %s", cont);
+		/* NOTREACHED */
 	    }
+
+	its_not_a_directive:
 	    if (*line == '#') {
 		/* If we're this far, the line must be a comment. */
 		goto nextLine;
Index: var.c
===================================================================
RCS file: /cvsroot/pkgsrc/bootstrap/bmake/var.c,v
retrieving revision 1.2
diff -u -p -r1.2 var.c
--- var.c	23 Aug 2004 03:44:34 -0000	1.2
+++ var.c	25 Aug 2005 10:32:39 -0000
@@ -148,6 +148,11 @@ static char	varNoError[] = "";
 GNode          *VAR_GLOBAL;   /* variables from the makefile */
 GNode          *VAR_CMD;      /* variables defined on the command-line */
 
+/* When non-zero, all assignments mark the destination variable read-only,
+ * so it cannot be modified again.
+ */
+static int var_readonly_mode = 0;
+
 #define FIND_CMD	0x1   /* look in VAR_CMD when searching */
 #define FIND_GLOBAL	0x2   /* look in VAR_GLOBAL as well */
 #define FIND_ENV  	0x4   /* look in the environment also */
@@ -166,6 +171,7 @@ typedef struct Var {
 #define VAR_KEEP	8	    /* Variable is VAR_JUNK, but we found
 				     * a use for it in some modifier and
 				     * the value is therefore valid */
+#define VAR_READONLY	16
 }  Var;
 
 
@@ -388,8 +394,13 @@ VarAdd (name, val, ctxt)
     h = Hash_CreateEntry (&ctxt->context, name, NULL);
     Hash_SetValue(h, v);
     v->name = h->name;
+
+    if (var_readonly_mode)
+	v->flags |= VAR_READONLY;
+
     if (DEBUG(VAR)) {
-	printf("%s:%s = %s\n", ctxt->name, name, val);
+	printf("%s:%s = %s%s\n", ctxt->name, name, val,
+	    (v->flags & VAR_READONLY) ? " (read-only)" : "");
     }
 }
 
@@ -421,6 +432,9 @@ Var_Delete(name, ctxt)
 	register Var 	  *v;
 
 	v = (Var *)Hash_GetValue(ln);
+	if (v->flags & VAR_READONLY) {
+	    Parse_Error(PARSE_FATAL, "%s: Readonly variables cannot be deleted.", v->name);
+	}
 	if (v->name != ln->name)
 		free(v->name);
 	Hash_DeleteEntry(&ctxt->context, ln);
@@ -429,6 +443,25 @@ Var_Delete(name, ctxt)
     }
 }
 
+void
+Var_Make_Readonly(char *name, GNode *ctxt)
+{
+	Hash_Entry *ln;
+	Var *v;
+
+	if (DEBUG(VAR)) {
+		printf("%s:set_readonly %s\n", ctxt->name, name);
+	}
+
+	ln = Hash_FindEntry(&ctxt->context, name);
+	if (ln == NULL) {
+		Parse_Error(PARSE_WARNING, ".readonly on undefined variables has no effect.");
+	} else {
+		v = (Var *)Hash_GetValue(ln);
+		v->flags |= VAR_READONLY;
+	}
+}
+
 /*-
  *-----------------------------------------------------------------------
  * Var_Set --
@@ -472,12 +505,18 @@ Var_Set (name, val, ctxt, flags)
     v = VarFind (name, ctxt, 0);
     if (v == (Var *) NIL) {
 	VarAdd (name, val, ctxt);
+    } else if (v->flags & VAR_READONLY) {
+	Parse_Error(PARSE_FATAL, "%s: Readonly variables cannot be modified.", v->name);
     } else {
 	Buf_Discard(v->val, Buf_Size(v->val));
 	Buf_AddBytes(v->val, strlen(val), (Byte *)val);
 
+	if (var_readonly_mode)
+	    v->flags |= VAR_READONLY;
+
 	if (DEBUG(VAR)) {
-	    printf("%s:%s = %s\n", ctxt->name, name, val);
+	    printf("%s:%s = %s%s\n", ctxt->name, name, val,
+		(v->flags & VAR_READONLY) ? " (read-only)" : "");
 	}
     }
     /*
@@ -535,13 +574,19 @@ Var_Append (name, val, ctxt)
 
     if (v == (Var *) NIL) {
 	VarAdd (name, val, ctxt);
+    } else if (v->flags & VAR_READONLY) {
+	Parse_Error(PARSE_FATAL, "%s: Readonly variables cannot be modified.", v->name);
     } else {
 	Buf_AddByte(v->val, (Byte)' ');
 	Buf_AddBytes(v->val, strlen(val), (Byte *)val);
 
+	if (var_readonly_mode)
+	    v->flags |= VAR_READONLY;
+
 	if (DEBUG(VAR)) {
-	    printf("%s:%s = %s\n", ctxt->name, name,
-		   (char *) Buf_GetAll(v->val, (int *)NULL));
+	    printf("%s:%s = %s%s\n", ctxt->name, name,
+		   (char *) Buf_GetAll(v->val, (int *)NULL),
+		   (v->flags & VAR_READONLY) ? " (read-only)" : "");
 	}
 
 	if (v->flags & VAR_FROM_ENV) {
@@ -2926,3 +2971,9 @@ Var_Dump (ctxt)
 	    VarPrintVar(Hash_GetValue(h));
     }
 }
+
+void
+Var_Set_Readonly_Mode(int onoff)
+{
+	var_readonly_mode = onoff;
+}

--------------050907040208000803020706--