[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5653: Remove CDN in a Box Makefile directory check

[GitBox <gi...@apache.org>]

ocket8888 commented on a change in pull request #5653:
URL: https://github.com/apache/trafficcontrol/pull/5653#discussion_r597918493



##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -105,43 +100,44 @@ endif
 
 # Actual output rpm recipies
 traffic_monitor/traffic_monitor.rpm: ../../dist/traffic_monitor-$(SPECIAL_SAUCE)
-	cp -f $? $@
+	cp -f "$(CIAB_DIR)/$?" "$(CIAB_DIR)/$@"

Review comment:
       This recipe is a lie. Using this recipe does not produce the target file, unless PWD === CIAB_DIR.
   
   You can get around that by making these recipes `.PHONY`, but that doesn't help with the problem this causes: `make` when run outside of CIAB_DIR will never conclude that the target doesn't need to be rebuilt - unless you happen to be doing it in a place where the target file actually _does_ coincidentally exist, in which case `make` will get confused and use the wrong information to arrive at its conclusions.
   
   You could also make the targets variables and then write recipes with that like
   
   ```make
   TM_RPM := $(CIAB_DIR)/traffic_monitor/traffic_monitor.rpm
   TM_DIST_RPM := $(TC_DIR)/dist/traffic_monitor-$(SPECIAL_SAUCE)
   $(TM_RPM): $(TM_DIST_RPM)
       cp -f $? $@
   
   $(TM_DIST_RPM): $(TM_SOURCE)
       "$(PKG_COMMAND)" $(PKG_FLAGS) traffic_monitor$(BUILD_SUFFIX)
   ```
   
   or you could still enforce the `make` command is run from the same directory as the `Makefile` but just fix the string manipulation to account for whitespace. I think that's perfectly acceptable
   $(TM_RPM): $(TC




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org