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

dependencies and tbprobe #2660

Closed
vondele opened this issue May 3, 2020 · 20 comments
Closed

dependencies and tbprobe #2660

vondele opened this issue May 3, 2020 · 20 comments

Comments

@vondele
Copy link
Member

vondele commented May 3, 2020

just to track (this is an old issue), the current Makefile/dependencies/etc are not correct for tbprobe.o i.e. if for example types.h is modified, tbprobe.cpp is not recompiled, potentially leading to bugs:

make clean && make -j ARCH=x86-64-modern build | grep tbprobe
touch types.h
make -j ARCH=x86-64-modern build | grep tbprobe

make clean or implicitly make profile-build can be used as a workaround.

@xoto10
Copy link
Contributor

xoto10 commented May 4, 2020

Yes, this is annoying isn't it. Maybe there is a more generic way, but this patch appears to fix the problem: master...xoto10:make1

@vondele
Copy link
Member Author

vondele commented May 5, 2020

@xoto10 yes that fixes it, but leaves the incorrect auto-generated dependencies (.depend file) still around, and moves from auto-generated to hand-maintained dependencies. I would prefer to either fix our auto-generation procedure (but didn't figure out how to do that cleanly), or move tbprobe in the main src directory, which would allow the current dependency generation to do the right thing.

@xoto10
Copy link
Contributor

xoto10 commented May 6, 2020

How about master...xoto10:make2 ?
The problem seems to be the rule in .depend says tbprobe.o: ... when we want it to say syzygy/tbprobe.o: ...
I tried to use a case statement to detect objects with subdirs but it wouldn't work for some reason I couldn't figure out so I used an if statement instead. The new code puts extra rules at the end of .depend with the subdir included, which seems to let make work properly :)

gvreuls added a commit to gvreuls/Stockfish that referenced this issue May 6, 2020
Changes:
- All .o object files now reside in the src directory itself.
- Instead of a single .depend file there now is a .dep directory
containing individual .d dependency files for every generated object
file.
- The src directory is added to the include path so source and header
files in subdirectories (like syzygy) can include header files in the
src directory directly (without the "../" prefix).

No functional change.
@gvreuls
Copy link
Contributor

gvreuls commented May 6, 2020

Here's my attempt. It doesn't require any manual processing of dependencies but there are some structural changes (see the first commit message): master...gvreuls:syzygy_dep

@vondele
Copy link
Member Author

vondele commented May 6, 2020

@gvreuls thanks, one more option :-). Will this also work for the MSVC setup (e.g. our CI) ?
The 'special' aspect (IIUC) is that it now compiles files from a different directory src/syzygy in src/

@gvreuls
Copy link
Contributor

gvreuls commented May 6, 2020

@vondele I can't test for MSVC but I could do a pull request to find out.

Placing the syzygy/ object files in src/ is necessary to get rid of the "syzygy/../someheader.h" references that break the dependency checks. The obvious solution would be to put the syzygy/ source files in src/ itself but since it's a fairly separate project I think it's better to put the syzygy/ directory in the make VPATH so it will be searched for tbprobe.cpp when it isn't found in src/

@vondele
Copy link
Member Author

vondele commented May 6, 2020

feel free to do a WIP PR to figure out.

@gvreuls
Copy link
Contributor

gvreuls commented May 6, 2020

@vondele I need to add src/ to the MSVC include path so I have to add either a /I . or a /I src to the compiler options but I have no idea where to do that for the CI.

@vondele
Copy link
Member Author

vondele commented May 6, 2020

Stockfish/appveyor.yml contains the setup.... but I don't know the details of it.

@xoto10
Copy link
Contributor

xoto10 commented May 6, 2020

Ah, seeing that idea helped me :)

Perhaps we're combining several ideas here. The smallest change to compile in the current dir seems to be: d931b46

Using -I. to avoid using ../ in includes seems like a good idea, adding that gives master...xoto10:make3)

@gvreuls
Copy link
Contributor

gvreuls commented May 6, 2020

@xoto10 If you do a PR you might want to add this Appveyor change to make the MSVC CI work. It adds the src directory to the MSVC include path.

@vondele
Copy link
Member Author

vondele commented May 6, 2020

I'm actually thinking that having the explicit path in the includes is a good thing, otherwise we have to add additional compiler flags to find them (and so might people reading the code). So maybe d931b46 is best ? (still need to figure out what that does exactly ;-) )

@xoto10
Copy link
Contributor

xoto10 commented May 6, 2020

:)
A key part for me was finding this reference: https://www.gnu.org/software/make/manual/make.html#Automatic-Variables
It's quite a while since I did anything with make (and gnu make is considerably more powerful), but seeing @gvreuls using $< made me look harder for the reference to what all these $@ type things mean :)

@vondele
Copy link
Member Author

vondele commented May 6, 2020

@xoto10 ... but now recompiles happen always even if up-to-date

@xoto10
Copy link
Contributor

xoto10 commented May 6, 2020

Ah, that's not good - let me look ...

@gvreuls
Copy link
Contributor

gvreuls commented May 6, 2020

You're writing the tbprobe.o object file to a different location from where OBJS specifies it, so it will be rebuilt every time because make thinks it doesn't exist. It's either that or specifying where tbprobe.o goes in OBJS and adding the syzygy directory to the VPATH like in my solution.

@xoto10
Copy link
Contributor

xoto10 commented May 6, 2020

Yes, I tried VPATH, but then the .depend build doesn't work because it can't find tbprobe.cpp (because it's syzygy/tbprobe.cpp).
This seems to work, I define SRCS instead of OBJS and rename from .cpp to .o in EXE rule instead of vice versa in .depend rule, and also use gnu $notdir to remove the dirname where appropriate: master...e49fd61

@gvreuls
Copy link
Contributor

gvreuls commented May 6, 2020

I've reduced my solution to the minimum too, it just changes the Makefile and doesn't add the src/ directory to the include path any longer. It still generates separate dependency files for each object file in the (hidden) .dep directory because this is more efficient (it saves a compiler run to generate the dependency file). See PR #2663

@xoto10
Copy link
Contributor

xoto10 commented May 6, 2020

I see all the make manuals seem to suggest creating a separate .d file for each source so it may make sense to go that way. I just tried to get the minimum change with the method we have now. Either way, it looks like we have some progress :)

@vondele
Copy link
Member Author

vondele commented May 8, 2020

@xoto10 and @gvreuls thanks for working on this issue!

I have a preference for the PR by @xoto10 and intend to merge that later today or this weekend.

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

No branches or pull requests

4 participants