Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix .testing Makefile with WORKSPACE #732

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

herrwang0
Copy link

When WORKSPACE is specified, the recursive make for target code (for regression tests) does not work properly. This PR fixes this bug.

@marshallward
Copy link
Member

marshallward commented Sep 29, 2024

Can you describe the problem you are seeing? I just compiled and ran the test with the current Makefile and following settings:

WORKSPACE = $HOME/xxx
DO_REGRESSION_TESTS = true

and everything passed.

(Note that WORKSPACE has been phased out in favor of BUILD and WORK, which decouples the build and run directories, but I still don't understand why we need to set WORKSPACE in this situation.)

(Edit: I also tried WORKSPACE = $HOME/a/b/c/d to check for a directory tree depth issue, but it still seems to work.)

@herrwang0
Copy link
Author

This is actually a bit tricky.
The problem occurred when I was using the command-line variables. So the variable WORKSPACE got passed to target's make call (line 285). The config.mk method does not have this problem, because, well, TARGET_CODEBASE directory does not have a config.mk, so WORKSPACE is always ..

@marshallward
Copy link
Member

because, well, TARGET_CODEBASE directory does not have a config.mk, so WORKSPACE is always ..

OK, that explains the problem. Thanks for working that out.

This is a very nice solution, and I'd suggest the following small modification:

 # Target codebase should use its own build system
 $(BUILD)/target/MOM6: $(BUILD)/target FORCE | $(TARGET_CODEBASE)
-       $(MAKE) -C $(TARGET_CODEBASE)/.testing build/symmetric/MOM6
+       $(MAKE) -C $(TARGET_CODEBASE)/.testing BUILD=build build/symmetric/MOM6
 
 $(BUILD)/target: | $(TARGET_CODEBASE)
+       $(MAKE) -C $(TARGET_CODEBASE)/.testing BUILD=build build/symmetric/
        ln -s $(abspath $(TARGET_CODEBASE))/.testing/build/symmetric $@

The first change is to replace WORKSPACE with BUILD. This is because we have phased out the WORKSPACE macro.

The second change also helps to prevent a dead build/target symlink by also build its target directory.

@herrwang0
Copy link
Author

@marshallward Thanks for the explanation! I modified the PR following the suggestion.

@marshallward
Copy link
Member

Oops! My "innocent" suggestion broke the MacOS tests. I think it's something about GNU vs BSD Make and how end-slashes are handled.

Anyway, let's leave that for another time. I think just using BUILD in place of WORKSPACE should be good enough. Sorry for making this more complicated than it needed to be.

This commit fixes a bug that target code cannot be built for regression
tests when BUILD is specified with command line arguments. The fix
explicitly specifies BUILD argument for target codebase, avoiding the
variable being passed from the parent make.
@herrwang0
Copy link
Author

Oops! My "innocent" suggestion broke the MacOS tests. I think it's something about GNU vs BSD Make and how end-slashes are handled.

Anyway, let's leave that for another time. I think just using BUILD in place of WORKSPACE should be good enough. Sorry for making this more complicated than it needed to be.

That's good know! Removing the end-slash and using build/symmetric seems to work. But I am not sure if that'd be the general solution (my rough understanding is BSD doesn't like end-slashes). Does .POSIX: do anything?

Anyway, I will go ahead without the mkdir -p fail-safe.

@marshallward
Copy link
Member

Copy link
Member

@marshallward marshallward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the additional revision.

.POSIX: could help with the problem above, but I don't think the slash usage is consistent enough in the file to get it all working, so probably best to just hold off and do a proper POSIX compatibility check some time in the future. (Or not... 🤷)

@marshallward marshallward merged commit 5dcc8e0 into NOAA-GFDL:dev/gfdl Oct 4, 2024
10 checks passed
@herrwang0 herrwang0 deleted the fix_testing_make branch October 7, 2024 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants