Merge branch 'develop' into feature/pkg-id-type

src/dds/build/deps.cpp

@@ -66,12 +66,11 @@ msvc_deps_info dds::parse_msvc_output_for_deps(std::string_view output, std::str
 }
 
 void dds::update_deps_info(database& db, const deps_info& deps) {
-    db.store_mtime(deps.output, fs::last_write_time(deps.output));
     db.store_file_command(deps.output, {deps.command, deps.command_output});
     db.forget_inputs_of(deps.output);
     for (auto&& inp : deps.inputs) {
-        db.store_mtime(inp, fs::last_write_time(inp));
-        db.record_dep(inp, deps.output);
+        auto mtime = fs::last_write_time(inp);
+        db.record_dep(inp, deps.output, mtime);
     }
 }
 
@@ -89,7 +88,7 @@ deps_rebuild_info dds::get_rebuild_info(database& db, path_ref output_path) {
     auto& inputs        = *inputs_;
     auto  changed_files =  //
         inputs             //
-        | ranges::views::filter([](const seen_file_info& input) {
+        | ranges::views::filter([](const input_file_info& input) {
               return !fs::exists(input.path) || fs::last_write_time(input.path) != input.last_mtime;
           })
         | ranges::views::transform([](auto& info) { return info.path; })  //

src/dds/build/plan/compile_exec.cpp

@@ -152,6 +152,7 @@ std::optional<deps_info> do_compile(const compile_file_full& cf, build_env_ref e
     }
 
     // We must always generate deps info if it was possible:
+    assert(compiled_okay);
     assert(ret_deps_info.has_value() || env.toolchain.deps_mode() == deps_mode::none);
     return ret_deps_info;
 }

src/dds/db/database.cpp

@@ -22,8 +22,17 @@ void migrate_1(sqlite3::database& db) {
     db.exec(R"(
         CREATE TABLE dds_files (
             file_id INTEGER PRIMARY KEY,
-            path TEXT NOT NULL UNIQUE,
-            mtime INTEGER NOT NULL
+            path TEXT NOT NULL UNIQUE
+        );
+        CREATE TABLE dds_file_commands (
+            command_id INTEGER PRIMARY KEY,
+            file_id
+                INTEGER
+                UNIQUE
+                NOT NULL
+                REFERENCES dds_files(file_id),
+            command TEXT NOT NULL,
+            output TEXT NOT NULL
         );
         CREATE TABLE dds_deps (
             input_file_id
@@ -34,18 +43,9 @@ void migrate_1(sqlite3::database& db) {
                 INTEGER
                 NOT NULL
                 REFERENCES dds_files(file_id),
+            input_mtime INTEGER NOT NULL,
             UNIQUE(input_file_id, output_file_id)
         );
-        CREATE TABLE dds_file_commands (
-            command_id INTEGER PRIMARY KEY,
-            file_id
-                INTEGER
-                UNIQUE
-                NOT NULL
-                REFERENCES dds_files(file_id),
-            command TEXT NOT NULL,
-            output TEXT NOT NULL
-        );
     )");
 }
 
@@ -106,72 +106,45 @@ database database::open(const std::string& db_path) {
 database::database(sqlite3::database db)
     : _db(std::move(db)) {}
 
-std::optional<fs::file_time_type> database::last_mtime_of(path_ref file_) {
+std::int64_t database::_record_file(path_ref path_) {
+    auto path = fs::weakly_canonical(path_);
+    sqlite3::exec(_stmt_cache(R"(
+                    INSERT OR IGNORE INTO dds_files (path)
+                    VALUES (?)
+                  )"_sql),
+                  std::forward_as_tuple(path.generic_string()));
     auto& st = _stmt_cache(R"(
-        SELECT mtime FROM dds_files WHERE path = ?
+        SELECT file_id
+          FROM dds_files
+         WHERE path = ?1
     )"_sql);
     st.reset();
-    auto path      = fs::weakly_canonical(file_);
-    st.bindings[1] = path.string();
-    auto maybe_res = sqlite3::unpack_single_opt<std::int64_t>(st);
-    if (!maybe_res) {
-        return std::nullopt;
-    }
-    auto [timestamp] = *maybe_res;
-    return fs::file_time_type(fs::file_time_type::duration(timestamp));
+    auto str       = path.generic_string();
+    st.bindings[1] = str;
+    auto [rowid]   = sqlite3::unpack_single<std::int64_t>(st);
+    return rowid;
 }
 
-void database::store_mtime(path_ref file, fs::file_time_type time) {
-    auto& st = _stmt_cache(R"(
-        INSERT INTO dds_files (path, mtime)
-        VALUES (?1, ?2)
-        ON CONFLICT(path) DO UPDATE SET mtime = ?2
+void database::record_dep(path_ref input, path_ref output, fs::file_time_type input_mtime) {
+    auto  in_id  = _record_file(input);
+    auto  out_id = _record_file(output);
+    auto& st     = _stmt_cache(R"(
+        INSERT OR IGNORE INTO dds_deps (input_file_id, output_file_id, input_mtime)
+        VALUES (?, ?, ?)
     )"_sql);
-    sqlite3::exec(st,
-                  std::forward_as_tuple(fs::weakly_canonical(file).string(),
-                                        time.time_since_epoch().count()));
-}
-
-void database::record_dep(path_ref input, path_ref output) {
-    auto& st = _stmt_cache(R"(
-        WITH input AS (
-            SELECT file_id
-              FROM dds_files
-             WHERE path = ?1
-        ),
-        output AS (
-            SELECT file_id
-              FROM dds_files
-             WHERE path = ?2
-        )
-        INSERT OR IGNORE INTO dds_deps (input_file_id, output_file_id)
-        VALUES (
-            (SELECT * FROM input),
-            (SELECT * FROM output)
-        )
-    )"_sql);
-    sqlite3::exec(st,
-                  std::forward_as_tuple(fs::weakly_canonical(input).string(),
-                                        fs::weakly_canonical(output).string()));
+    sqlite3::exec(st, std::forward_as_tuple(in_id, out_id, input_mtime.time_since_epoch().count()));
 }
 
 void database::store_file_command(path_ref file, const command_info& cmd) {
+    auto file_id = _record_file(file);
+
     auto& st = _stmt_cache(R"(
-        WITH file AS (
-            SELECT file_id
-            FROM dds_files
-            WHERE path = ?1
-        )
         INSERT OR REPLACE
           INTO dds_file_commands(file_id, command, output)
-        VALUES (
-            (SELECT * FROM file),
-            ?2,
-            ?3
-        )
+        VALUES (?1, ?2, ?3)
     )"_sql);
     sqlite3::exec(st,
-                  std::forward_as_tuple(fs::weakly_canonical(file).string(),
+                  std::forward_as_tuple(file_id,
                                         std::string_view(cmd.command),
                                         std::string_view(cmd.output)));
 }
@@ -189,31 +162,27 @@ void database::forget_inputs_of(path_ref file) {
     sqlite3::exec(st, std::forward_as_tuple(fs::weakly_canonical(file).string()));
 }
 
-std::optional<std::vector<seen_file_info>> database::inputs_of(path_ref file_) {
+std::optional<std::vector<input_file_info>> database::inputs_of(path_ref file_) {
     auto  file = fs::weakly_canonical(file_);
     auto& st   = _stmt_cache(R"(
         WITH file AS (
             SELECT file_id
               FROM dds_files
              WHERE path = ?
-        ),
-        input_ids AS (
-            SELECT input_file_id
-              FROM dds_deps
-             WHERE output_file_id IN file
         )
-        SELECT path, mtime
-          FROM dds_files
-         WHERE file_id IN input_ids
+        SELECT path, input_mtime
+          FROM dds_deps
+          JOIN dds_files ON input_file_id = file_id
+         WHERE output_file_id IN file
     )"_sql);
     st.reset();
     st.bindings[1] = file.string();
     auto tup_iter  = sqlite3::iter_tuples<std::string, std::int64_t>(st);
 
-    std::vector<seen_file_info> ret;
+    std::vector<input_file_info> ret;
     for (auto& [path, mtime] : tup_iter) {
         ret.emplace_back(
-            seen_file_info{path, fs::file_time_type(fs::file_time_type::duration(mtime))});
+            input_file_info{path, fs::file_time_type(fs::file_time_type::duration(mtime))});
     }
 
     if (ret.empty()) {

src/dds/db/database.hpp

@@ -20,7 +20,7 @@ struct command_info {
     std::string output;
 };
 
-struct seen_file_info {
+struct input_file_info {
     fs::path           path;
     fs::file_time_type last_mtime;
 };
@@ -33,6 +33,8 @@ class database {
     explicit database(neo::sqlite3::database db);
     database(const database&) = delete;
 
+    std::int64_t _record_file(path_ref p);
+
 public:
     static database open(const std::string& db_path);
     static database open(path_ref db_path) { return open(db_path.string()); }
@@ -43,13 +45,11 @@ public:
         return neo::sqlite3::transaction_guard(_db);
     }
 
-    std::optional<fs::file_time_type> last_mtime_of(path_ref file);
-    void                              store_mtime(path_ref file, fs::file_time_type time);
-    void                              record_dep(path_ref input, path_ref output);
-    void                              store_file_command(path_ref file, const command_info& cmd);
-    void                              forget_inputs_of(path_ref file);
+    void record_dep(path_ref input, path_ref output, fs::file_time_type input_mtime);
+    void store_file_command(path_ref file, const command_info& cmd);
+    void forget_inputs_of(path_ref file);
 
-    std::optional<std::vector<seen_file_info>> inputs_of(path_ref file);
+    std::optional<std::vector<input_file_info>> inputs_of(path_ref file);
     std::optional<command_info>                command_of(path_ref file);
 };
 

src/dds/db/database.test.cpp

@@ -5,17 +5,3 @@
 using namespace std::literals;
 
 TEST_CASE("Create a database") { auto db = dds::database::open(":memory:"s); }
-TEST_CASE("Read an absent file's mtime") {
-    auto db        = dds::database::open(":memory:"s);
-    auto mtime_opt = db.last_mtime_of("bad/file/path");
-    CHECK_FALSE(mtime_opt.has_value());
-}
-
-TEST_CASE("Record a file") {
-    auto db = dds::database::open(":memory:"s);
-    auto time = dds::fs::file_time_type::clock::now();
-    db.store_mtime("file/something", time);
-    auto mtime_opt = db.last_mtime_of("file/something");
-    REQUIRE(mtime_opt.has_value());
-    CHECK(mtime_opt == time);
-}

src/dds/toolchain/toolchain.cpp

@@ -139,6 +139,11 @@ std::optional<toolchain> toolchain::get_builtin(std::string_view tc_id) noexcept
 
     std::string tc_content;
 
+    if (starts_with(tc_id, "debug:")) {
+        tc_id = tc_id.substr("debug:"sv.length());
+        tc_content += "Debug: True\n";
+    }
+
     if (starts_with(tc_id, "ccache:")) {
         tc_id = tc_id.substr("ccache:"sv.length());
         tc_content += "Compiler-Launcher: ccache\n";

tests/db/project/src/1.cpp

@@ -0,0 +1,3 @@
+#include "./values.hpp"
+
+int value_1() { return first_value; }

tests/db/project/src/2.cpp

@@ -0,0 +1,3 @@
+#include "./values.hpp"
+
+int value_2() { return second_value; }

tests/db/project/src/app.main.cpp

@@ -0,0 +1,5 @@
+#include "./foo.hpp"
+
+#include <cmath>
+
+int main() { return std::abs(value_1() - value_2()); }

tests/db/project/src/foo.hpp

@@ -0,0 +1,4 @@
+#pragma once
+
+int value_1();
+int value_2();

tests/db/project/src/values.hpp

@@ -0,0 +1,4 @@
+#pragma once
+
+const int first_value  = 32;
+const int second_value = 32;

tests/db/test_compile_deps.py

@@ -0,0 +1,106 @@
+import subprocess
+import time
+
+import pytest
+
+from tests import dds, DDS, dds_fixture_conf_1
+from dds_ci import proc
+
+## #############################################################################
+## #############################################################################
+## The test project in this directory contains a single application and two
+## functions, each defined in a separate file. The two functions each return
+## an integer, and the application exit code will be the difference between
+## the two integers. (They are passed through std::abs(), so it is always a
+## positive integer). The default value is 32 in both functions.
+## #############################################################################
+## The purpose of these tests is to ensure the reliability of the compilation
+## dependency database. Having a miscompile because there was a failure to
+## detect file changes is a catastrophic bug!
+
+
+def build_and_get_rc(dds: DDS) -> int:
+    dds.build()
+    app = dds.build_dir / ('app' + dds.exe_suffix)
+    return proc.run(app).returncode
+
+
+def test_simple_rebuild(dds: DDS):
+    """
+    Check that changing a source file will update the resulting application.
+    """
+    assert build_and_get_rc(dds) == 0
+    dds.scope.enter_context(
+        dds.set_contents(
+            'src/1.cpp',
+            b'''
+            int value_1() { return 33; }
+            ''',
+        ))
+    # 33 - 32 = 1
+    assert build_and_get_rc(dds) == 1
+
+
+def test_rebuild_header_change(dds: DDS):
+    """Change the content of the header which defines the values"""
+    assert build_and_get_rc(dds) == 0
+    dds.scope.enter_context(
+        dds.set_contents(
+            'src/values.hpp',
+            b'''
+            const int first_value = 63;
+            const int second_value = 88;
+            ''',
+        ))
+    assert build_and_get_rc(dds) == (88 - 63)
+
+
+def test_partial_build_rebuild(dds: DDS):
+    """
+    Change the content of a header, but cause one user of that header to fail
+    compilation. The fact that compilation fails means it is still `out-of-date`,
+    and will need to be compiled after we have fixed it up.
+    """
+    assert build_and_get_rc(dds) == 0
+    dds.scope.enter_context(
+        dds.set_contents(
+            'src/values.hpp',
+            b'''
+            const int first_value_q  = 6;
+            const int second_value_q = 99;
+            ''',
+        ))
+    # Header now causes errors in 1.cpp and 2.cpp
+    with pytest.raises(subprocess.CalledProcessError):
+        dds.build()
+    # Fix 1.cpp
+    dds.scope.enter_context(
+        dds.set_contents(
+            'src/1.cpp',
+            b'''
+            #include "./values.hpp"
+
+            int value_1() { return first_value_q; }
+            ''',
+        ))
+    # We will still see a failure, but now the DB will record the updated values.hpp
+    with pytest.raises(subprocess.CalledProcessError):
+        dds.build()
+
+    # Should should raise _again_, even though we've successfully compiled one
+    # of the two files with the changed `values.hpp`, because `2.cpp` still
+    # has a pending update
+    with pytest.raises(subprocess.CalledProcessError):
+        dds.build()
+
+    dds.scope.enter_context(
+        dds.set_contents(
+            'src/2.cpp',
+            b'''
+            #include "./values.hpp"
+
+            int value_2() { return second_value_q; }
+            ''',
+        ))
+    # We should now compile and link to get the updated value
+    assert build_and_get_rc(dds) == (99 - 6)