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

Ninja: use header dependencies for better rebuild support #3769

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

lieser
Copy link
Collaborator

@lieser lieser commented Oct 23, 2023

When locally developing, currently the build system does not properly track changes to headers.
This can result in broken rebuilds, that are currently often workaround by re-invoking ./configure.py before a rebuild.

Ninja supports for gcc, clang and msvc to track the header dependencies (see https://ninja-build.org/manual.html#ref_headers for more details).
This PR enables usage of the header dependencies feature then using ninja.
When changing headers this can greatly improve the rebuild because only ninja needs to be invoked, and a new call to ./configure.py is not needed.

A note about Windows:
This only works with hard links, as ninja will fail to detect a new modification time of the target file.
For hard links there seems to be also some caching involved, and when modifying the file under src/ and not build/, it sometimes can take a while for ninja to detect the change.

@lieser
Copy link
Collaborator Author

lieser commented Oct 23, 2023

Opened as a draft as I'm not sure that adding the logic to build.ninja is the best approach, or if it would be better to move more into configure.py and/or the compiler information under src/build-data/cc/.

@lieser lieser requested a review from reneme October 23, 2023 14:53
@lieser lieser force-pushed the pl/feature-ninja_header_deps branch from 07b8806 to c0fdeb6 Compare October 23, 2023 15:23
@reneme
Copy link
Collaborator

reneme commented Oct 23, 2023

This works well for me and actually helps with one of the bigger pains I have with Botan's build system. 😍

I'm not sure that adding the logic to build.ninja is the best approach, or if it would be better to move more into configure.py and/or the compiler information under src/build-data/cc/.

I would indeed suggest to add the compiler flags to src/build-data/cc/*.txt.
That would reduce the complexity in the ninja.in file quite a bit, I think. Not sure how to handle the difference in the invocations, though. The -MF $out.d seems to be necessary, despite it being the default anyway.

Edit: Could we create another "abstract" ninja rule, like:

rule compile_something

that handles the depfile and deps and let compile_exe, compile_example_exe and compile_lib simply call this? At least it would be less repetition then!?

@coveralls
Copy link

coveralls commented Oct 23, 2023

Coverage Status

coverage: 91.713% (+0.008%) from 91.705% when pulling f29366a on pl/feature-ninja_header_deps into 3a02016 on master.

@randombit
Copy link
Owner

I would indeed suggest to add the compiler flags to src/build-data/cc/*.txt.

Strong 👍 on this aspect - ideally we should not be anywhere asking "is this compiler MSVC" or "is this compiler GCC" but instead "does the compiler have the capability X, and if so how do I invoke it". Then later someone can update the compiler info for say Intel C++, and then everything Just Works without any additional changes.

@lieser
Copy link
Collaborator Author

lieser commented Oct 23, 2023

I would indeed suggest to add the compiler flags to src/build-data/cc/*.txt

The problem I see here is that for gcc/clang the file argument $out.d to -MF is ninja specific. I am a little hesitant to add something build tool specific to this files, but maybe still the best solution.

Could we create another "abstract" ninja rule, like:

Don't know much about ninja, but from what I read in the specification I don't see any way for rules to kind of inherit properties from each other.

We could try to use top level variables to get rid of some repetition.

%{if compiler_is_gcc_or_clang}
write_dependencies_flags = -MD -MF $out.d
dep_file =$out.d
%{endif}
%{if compiler_is_msvc}
write_dependencies_flags = /showIncludes
dep_file = 
%{endif}

rule compile_lib
  depfile = $dep_file
  deps = %{compiler}
  command = %{cxx} $write_dependencies_flags ...

But a few thinks are unclear:

  • Would an empty depfile = cause problems for msvc? My guess would be no (and a small test so far confirms it)
  • If we specify a ninja variable write_dependencies_flags = -MD -MF $out.d that is used in the command, how would the $out.d variable in it behave? Same question for dep_file/depfile.

@reneme reneme force-pushed the pl/feature-ninja_header_deps branch from 55ee6bc to 564edde Compare October 24, 2023 11:55
reneme and others added 3 commits October 24, 2023 14:07
This concatenates the static value behind the colon with the
value of the template variable. If this value is not set (aka ''),
also the static concatenated value is omitted.

Co-Authored-By: Philippe Lieser <[email protected]>
@reneme reneme force-pushed the pl/feature-ninja_header_deps branch from 564edde to f29366a Compare October 24, 2023 12:10
@lieser lieser marked this pull request as ready for review October 24, 2023 12:23
@reneme
Copy link
Collaborator

reneme commented Oct 24, 2023

@lieser and I did have another look at this and came up with a reasonable solution. To keep the templates readable, and not clutter them with %{if}...%{endif} blocks too much, we added a new macro: %{somevar|concat:some_constant_value}. The pull request contains an explanation on how to use this in detail in doc/dev_ref/configure.rst.

The configuration for the various compilers is now steered fully from the src/build-data/cc/*.txt files.

@reneme reneme requested a review from randombit October 24, 2023 14:57
@lieser lieser added this to the Botan 3.3.0 milestone Oct 24, 2023
@reneme reneme merged commit 4fbff62 into master Oct 26, 2023
@reneme reneme deleted the pl/feature-ninja_header_deps branch October 26, 2023 08:47
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.

4 participants