Subtle fix to nested builds rebuilding

* Add labels to hot-loading code due to its use of c-linkage, which
caused rebuilds
* Name some more executables to prevent unnecessary re-links
* Make all CRCs track whether they were updated by their instance of
cakelisp. This is necessary because a Cakelisp instance could run
another cakelisp inside of it, then that Cakelisp could end up
updating the cache which the outer one also needs.
* Fix typo in test

.gitignore

@@ -49,6 +49,8 @@ test/SimpleMacros
 test/Defer
 test/Export
 test/Tutoral_Basics
+test/buildOptions
+test/runTests
 
 *.cake.cpp
 *.cake.hpp

runtime/HotReloading.cake

@@ -1,4 +1,7 @@
 (set-cakelisp-option use-c-linkage true)
+;; Because we could be building things without using C linkage, we need to separate all this code
+;; out to prevent oscillating rebuilds
+(add-build-config-label "HotLoadingModifiedCode")
 
 (import "DynamicLoader.cake"
         "ComptimeHelpers.cake" "CHelpers.cake")

runtime/HotReloadingCodeModifier.cake

@@ -31,6 +31,10 @@
 ;; This needs to be set before any functions which need C linkage are evaluated by Cakelisp
 (set-cakelisp-option use-c-linkage true)
 
+;; Because we could be building things without using C linkage, we need to separate all this code
+;; out to prevent oscillating rebuilds
+(add-build-config-label "HotLoadingModifiedCode")
+
 ;; This is a hack: We want to generate HotReloading.cake.hpp, but we don't want to inherit all the
 ;; loader-specific options. HotReloading.cake will see this define and not add the options
 ;; TODO: Make a context variable for preventing environment changes during &decls-only?

src/Build.cpp

@@ -472,7 +472,7 @@ static void buildWriteCacheFile(const char* buildOutputDir, ArtifactCrcTable& ca
 			outputTokens.push_back(artifactName);
 
 			Token crcToken = {
-			    TokenType_Symbol, std::to_string(crcPair.second), "Build.cpp", 1, 0, 0};
+			    TokenType_Symbol, std::to_string(crcPair.second.crc), "Build.cpp", 1, 0, 0};
 			outputTokens.push_back(crcToken);
 
 			outputTokens.push_back(closeParen);
@@ -488,7 +488,7 @@ static void buildWriteCacheFile(const char* buildOutputDir, ArtifactCrcTable& ca
 		    TokenType_String, std::to_string(crcPair.first), "Build.cpp", 1, 0, 0};
 		outputTokens.push_back(sourceArtifactName);
 
-		Token crcToken = {TokenType_Symbol, std::to_string(crcPair.second), "Build.cpp", 1, 0, 0};
+		Token crcToken = {TokenType_Symbol, std::to_string(crcPair.second.crc), "Build.cpp", 1, 0, 0};
 		outputTokens.push_back(crcToken);
 
 		outputTokens.push_back(closeParen);
@@ -559,19 +559,22 @@ bool buildReadCacheFile(const char* buildOutputDir, ArtifactCrcTable& cachedComm
 
 			if (invocationToken.contents.compare("command-crc") == 0)
 			{
-				cachedCommandCrcs[(*tokens)[keyIndex].contents] =
-				    static_cast<uint32_t>(std::stoul((*tokens)[crcIndex].contents));
+				CrcWithFlags loadedCrc = {
+				    static_cast<uint32_t>(std::stoul((*tokens)[crcIndex].contents)), false};
+				cachedCommandCrcs[(*tokens)[keyIndex].contents] = loadedCrc;
 			}
 			else if (invocationToken.contents.compare("header-crc") == 0)
 			{
-				headerCrcCache[(*tokens)[keyIndex].contents] =
-				    static_cast<uint32_t>(std::stoul((*tokens)[crcIndex].contents));
+				CrcWithFlags loadedCrc = {
+				    static_cast<uint32_t>(std::stoul((*tokens)[crcIndex].contents)), false};
+				headerCrcCache[(*tokens)[keyIndex].contents] = loadedCrc;
 			}
 			else if (invocationToken.contents.compare("source-artifact-crc") == 0)
 			{
+				CrcWithFlags loadedCrc = {
+				    static_cast<uint32_t>(std::stoul((*tokens)[crcIndex].contents)), false};
 				sourceArtifactFileCrcs[static_cast<uint32_t>(
-				    std::stoul((*tokens)[keyIndex].contents))] =
-				    static_cast<uint32_t>(std::stoul((*tokens)[crcIndex].contents));
+				    std::stoul((*tokens)[keyIndex].contents))] = loadedCrc;
 			}
 			else
 			{
@@ -601,15 +604,24 @@ void buildReadMergeWriteCacheFile(const char* buildOutputDir, ArtifactCrcTable&
 	buildReadCacheFile(buildOutputDir, mergedCachedCommandCrcs, mergedSourceArtifactFileCrcs,
 	                   mergedLoadedHeaderCrcCache);
 
-	// Merge, using our version as latest
+	// Merge, using our version as latest if we modified any CRCs
+	// Otherwise, we want to use the most recently read version in case a nested cakelisp ran and
+	// modified things.
 	for (ArtifactCrcTablePair& crcPair : newCommandCrcs)
-		mergedCachedCommandCrcs[crcPair.first] = crcPair.second;
+	{
+		if (crcPair.second.wasModified)
+			mergedCachedCommandCrcs[crcPair.first] = crcPair.second;
+	}
 	for (const HashedSourceArtifactCrcTablePair& crcPair : sourceArtifactFileCrcs)
 	{
-		mergedSourceArtifactFileCrcs[crcPair.first] = crcPair.second;
+		if (crcPair.second.wasModified)
+			mergedSourceArtifactFileCrcs[crcPair.first] = crcPair.second;
 	}
 	for (ArtifactCrcTablePair& crcPair : changedHeaderCrcCache)
-		mergedLoadedHeaderCrcCache[crcPair.first] = crcPair.second;
+	{
+		if (crcPair.second.wasModified)
+			mergedLoadedHeaderCrcCache[crcPair.first] = crcPair.second;
+	}
 
 	buildWriteCacheFile(buildOutputDir, mergedCachedCommandCrcs, newCommandCrcs,
 	                    mergedSourceArtifactFileCrcs, mergedLoadedHeaderCrcCache);
@@ -767,14 +779,18 @@ static bool AreIncludedHeadersModified_Recursive(const std::vector<std::string>&
 		ArtifactCrcTable::iterator findIt = loadedHeaderCrcCache.find(filename);
 		if (findIt != loadedHeaderCrcCache.end())
 		{
-			bool isCrcChanged = (findIt->second != crc);
+			bool isCrcChanged = (findIt->second.crc != crc);
 			headerCrcDiffersFromExpected |= isCrcChanged;
 			// We only want to make an entry if we no longer match
 			if (isCrcChanged)
 			{
-				changedHeaderCrcCache[filename] = crc;
+				CrcWithFlags newCrc = {0};
+				newCrc.crc = crc;
+				newCrc.wasModified = true;
+				changedHeaderCrcCache[filename] = newCrc;
 				if (logging.includeScanning)
-					Logf("   >>> Header %s crc %u no longer matches %u.\n", filename, crc, findIt->second);
+					Logf("   >>> Header %s crc %u no longer matches %u.\n", filename, crc,
+					     findIt->second.crc);
 			}
 		}
 		else
@@ -782,7 +798,10 @@ static bool AreIncludedHeadersModified_Recursive(const std::vector<std::string>&
 			// We don't know anything about this header yet; we must assume it has "changed" and we
 			// need to rebuild whatever is dependent on it
 			headerCrcDiffersFromExpected |= true;
-			changedHeaderCrcCache[filename] = crc;
+			CrcWithFlags newCrc = {0};
+			newCrc.crc = crc;
+			newCrc.wasModified = true;
+			changedHeaderCrcCache[filename] = newCrc;
 			if (logging.includeScanning)
 				Logf("   >>> Header %s was unknown. Marking as changed.\n", filename);
 		}
@@ -800,14 +819,14 @@ static bool AreIncludedHeadersModified_Recursive(const std::vector<std::string>&
 
 // commandArguments should have terminating null sentinel
 bool commandEqualsCachedCommand(ArtifactCrcTable& cachedCommandCrcs, const char* artifactKey,
-                                const char** commandArguments, uint32_t* crcOut)
+                                const char** commandArguments, CrcWithFlags* crcOut)
 {
-	uint32_t newCommandCrc = 0;
+	CrcWithFlags newCommandCrc = {0};
 	if (logging.commandCrcs)
 		Log("\"");
 	for (const char** currentArg = commandArguments; *currentArg; ++currentArg)
 	{
-		crc32(*currentArg, strlen(*currentArg), &newCommandCrc);
+		crc32(*currentArg, strlen(*currentArg), &newCommandCrc.crc);
 		if (logging.commandCrcs)
 			Logf("%s ", *currentArg);
 	}
@@ -821,14 +840,17 @@ bool commandEqualsCachedCommand(ArtifactCrcTable& cachedCommandCrcs, const char*
 	if (findIt == cachedCommandCrcs.end())
 	{
 		if (logging.commandCrcs)
-			Logf("CRC32 for %s: %u (not cached)\n", artifactKey, newCommandCrc);
+			Logf("CRC32 for %s: %u (not cached)\n", artifactKey, newCommandCrc.crc);
+		crcOut->wasModified = true;
 		return false;
 	}
 
 	if (logging.commandCrcs)
-		Logf("CRC32 for %s: old %u new %u\n", artifactKey, findIt->second, newCommandCrc);
+		Logf("CRC32 for %s: old %u new %u\n", artifactKey, findIt->second.crc, newCommandCrc.crc);
 
-	return findIt->second == newCommandCrc;
+	bool commandEqualsCached = findIt->second.crc == newCommandCrc.crc;
+	crcOut->wasModified = !commandEqualsCached;
+	return commandEqualsCached;
 }
 
 bool cppFileNeedsBuild(EvaluatorEnvironment& environment, const char* sourceFilename,
@@ -837,7 +859,7 @@ bool cppFileNeedsBuild(EvaluatorEnvironment& environment, const char* sourceFile
                        HeaderModificationTimeTable& headerModifiedCache,
                        std::vector<std::string>& headerSearchDirectories)
 {
-	uint32_t commandCrc = 0;
+	CrcWithFlags commandCrc = {0};
 	bool commandEqualsCached = commandEqualsCachedCommand(cachedCommandCrcs, artifactFilename,
 	                                                      commandArguments, &commandCrc);
 	// We could avoid doing this work, but it makes it easier to log if we do it regardless of

src/Build.hpp

@@ -66,14 +66,22 @@ CAKELISP_API void makeTargetPlatformVersionArgument(char* resolvedArgumentOut,
 
 typedef std::unordered_map<std::string, FileModifyTime> HeaderModificationTimeTable;
 
+struct CrcWithFlags
+{
+	uint32_t crc;
+	// Track whether we actually changed the state of whatever the CRC is pointing to, as opposed to
+	// just loaded it and saved it back out
+	bool wasModified;
+};
+
 // If an existing cached build was run, check the current build's commands against the previous
 // commands via CRC comparison. This ensures changing commands will cause rebuilds
-typedef std::unordered_map<std::string, uint32_t> ArtifactCrcTable;
-typedef std::pair<const std::string, uint32_t> ArtifactCrcTablePair;
+typedef std::unordered_map<std::string, CrcWithFlags> ArtifactCrcTable;
+typedef std::pair<const std::string, CrcWithFlags> ArtifactCrcTablePair;
 
-// Uses a hash of the artifact name, then the source name
-typedef std::unordered_map<uint32_t, uint32_t> HashedSourceArtifactCrcTable;
-typedef std::pair<uint32_t, uint32_t> HashedSourceArtifactCrcTablePair;
+// Uses a hash of the artifact name, then the source name as key
+typedef std::unordered_map<uint32_t, CrcWithFlags> HashedSourceArtifactCrcTable;
+typedef std::pair<uint32_t, CrcWithFlags> HashedSourceArtifactCrcTablePair;
 
 // Why read, merge, write? Because it's possible we ran another instance of cakelisp in the same
 // directory during our build phase. The caches are shared state, so we don't want to blow away
@@ -90,7 +98,7 @@ bool buildReadCacheFile(const char* buildOutputDir, ArtifactCrcTable& cachedComm
 
 // commandArguments should have terminating null sentinel
 bool commandEqualsCachedCommand(ArtifactCrcTable& cachedCommandCrcs, const char* artifactKey,
-                                const char** commandArguments, uint32_t* crcOut);
+                                const char** commandArguments, CrcWithFlags* crcOut);
 
 struct EvaluatorEnvironment;
 

src/Evaluator.cpp

@@ -2049,11 +2049,14 @@ void resetGeneratorOutput(GeneratorOutput& output)
 	output.imports.clear();
 }
 
-uint32_t cacheUpdateFileCrc(EvaluatorEnvironment& environment, const char* filename)
+CrcWithFlags cacheUpdateFileCrc(EvaluatorEnvironment& environment, const char* filename)
 {
 	uint32_t sourceCrc = getFileCrc32(filename);
-	environment.cachedIntraBuildFileCrcs[filename] = sourceCrc;
-	return sourceCrc;
+	CrcWithFlags newCrc = {0};
+	newCrc.crc = sourceCrc;
+	newCrc.wasModified = true;
+	environment.cachedIntraBuildFileCrcs[filename] = newCrc;
+	return newCrc;
 }
 
 uint32_t getSourceArtifactKey(const char* source,
@@ -2069,7 +2072,7 @@ uint32_t getSourceArtifactKey(const char* source,
 void setSourceArtifactCrc(EvaluatorEnvironment& environment, const char* source,
                           const char* artifact)
 {
-	uint32_t sourceCrc = 0;
+	CrcWithFlags sourceCrc = {0};
 	{
 		ArtifactCrcTable::iterator findIt = environment.cachedIntraBuildFileCrcs.find(source);
 		if (findIt != environment.cachedIntraBuildFileCrcs.end())
@@ -2105,7 +2108,7 @@ bool crcsMatchExpectedUpdateCrcPairing(EvaluatorEnvironment& environment, const
 	}
 	else
 	{
-		uint32_t sourceCrc = 0;
+		CrcWithFlags sourceCrc = {0};
 		{
 			ArtifactCrcTable::iterator findIt = environment.cachedIntraBuildFileCrcs.find(source);
 			if (findIt != environment.cachedIntraBuildFileCrcs.end())
@@ -2113,7 +2116,7 @@ bool crcsMatchExpectedUpdateCrcPairing(EvaluatorEnvironment& environment, const
 			else
 				sourceCrc = cacheUpdateFileCrc(environment, source);
 		}
-		bool sourceCrcMatchesLastUse = sourceCrc == findIt->second;
+		bool sourceCrcMatchesLastUse = sourceCrc.crc == findIt->second.crc;
 		if (logging.buildReasons)
 		{
 			if (sourceCrcMatchesLastUse)
@@ -2123,7 +2126,7 @@ bool crcsMatchExpectedUpdateCrcPairing(EvaluatorEnvironment& environment, const
 			}
 			else
 				Logf("Artifact %s needs to build because source %s CRC is now %u (expected %u)\n",
-				     artifact, source, sourceCrc, findIt->second);
+				     artifact, source, sourceCrc.crc, findIt->second.crc);
 		}
 		return sourceCrcMatchesLastUse;
 	}

src/ModuleManager.cpp

@@ -1479,7 +1479,7 @@ bool moduleManagerLink(ModuleManager& manager, std::vector<BuildObject*>& buildO
 			return false;
 		}
 
-		uint32_t commandCrc = 0;
+		CrcWithFlags commandCrc = {0};
 		bool commandEqualsCached = commandEqualsCachedCommand(
 		    manager.cachedCommandCrcs, finalOutputName.c_str(), linkArgumentList, &commandCrc);
 

test/BuildOptions.cake

@@ -5,4 +5,6 @@
 
 (add-c-search-directory-module "src" "notsrc")
 
-(add-build-options "-Wall" "-Wextra" "-Wno-unused-parameter" "-O0")
+(add-build-options "-Wall" "-Wextra" "-Wno-unused-parameter" "-O1")
+
+(set-cakelisp-option executable-output "test/buildOptions")

test/Defer.cake

@@ -8,7 +8,7 @@
   (defer
     (free buffer)
     (fprintf stderr "Freed buffer\n"))
-  (defer (fprintf stderr "I could have dependend on buffer in my defer\n"))
+  (defer (fprintf stderr "I could have depended on buffer in my defer\n"))
   (each-in-range 5 i
     (var-cast-to another-buffer (addr char) (malloc 1024))
     (defer

test/RunTests.cake

@@ -14,7 +14,7 @@
      (array "Hello" "test/Hello.cake")
      (array "Macros" "test/SimpleMacros.cake")
      (array "Code modification" "test/CodeModification.cake")
-     ;; (array "Build options" "test/BuildOptions.cake")
+     (array "Build options" "test/BuildOptions.cake")
      (array "Execute" "test/Execute.cake")
      (array "Export" "test/ExportTest.cake")
      (array "Defines" "test/Defines.cake")
@@ -65,3 +65,5 @@
   (Log "\nRunTests: All tests succeeded!\n")
   (return true))
 (add-compile-time-hook-module pre-build run-tests)
+
+(set-cakelisp-option executable-output "test/runTests")