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

build: support lib.cc #1035

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 82 additions & 14 deletions src/BuildConfig.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ operator<<(std::ostream& os, VarType type) {

BuildConfig::BuildConfig(const std::string& packageName, const bool isDebug)
: packageName{ packageName }, isDebug{ isDebug } {
Comment on lines 65 to 66
Copy link
Member

Choose a reason for hiding this comment

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

Now this line looks awkward. Can you also please move this to the constructor body?

Suggested change
BuildConfig::BuildConfig(const std::string& packageName, const bool isDebug)
: packageName{ packageName }, isDebug{ isDebug } {
BuildConfig::BuildConfig(const std::string& packageName, const bool isDebug) {
packageName = packageName;
isDebug = isDebug;

libName = fmt::format("lib{}.a", packageName);
const fs::path projectBasePath = getProjectBasePath();
if (isDebug) {
outBasePath = projectBasePath / "poac-out" / "debug";
Expand Down Expand Up @@ -489,11 +490,22 @@ void
BuildConfig::defineLinkTarget(
const std::string& binTarget, const std::unordered_set<std::string>& deps
) {
std::vector<std::string> commands;
commands.emplace_back("$(CXX) $(CXXFLAGS) $^ $(LIBS) -o $@");
const std::vector<std::string> commands = {
"$(CXX) $(CXXFLAGS) $^ $(LIBS) -o $@"
};
defineTarget(binTarget, commands, deps);
}

void
BuildConfig::defineLibTarget(
const std::string& libTarget, const std::unordered_set<std::string>& deps
) {
const std::vector<std::string> commands = {
fmt::format("ar rcs {} $^", libName)
};
defineTarget(libTarget, commands, deps);
}

// Map a path to header file to the corresponding object file.
//
// e.g., src/path/to/header.h -> poac.d/path/to/header.o
Expand Down Expand Up @@ -791,6 +803,9 @@ BuildConfig::configureBuild() {
const auto isMainSource = [](const fs::path& file) {
return file.filename().stem() == "main";
};
const auto isLibSource = [](const fs::path& file) {
return file.filename().stem() == "lib";
};
fs::path mainSource;
for (const auto& entry : fs::directory_iterator(srcDir)) {
const fs::path& path = entry.path();
Expand All @@ -802,13 +817,34 @@ BuildConfig::configureBuild() {
}
if (mainSource.empty()) {
mainSource = path;
executable = true;
} else {
throw PoacError("multiple main sources were found");
}
}

if (mainSource.empty()) {
throw PoacError(fmt::format("src/main{} was not found", SOURCE_FILE_EXTS));
fs::path libSource;
for (const auto& entry : fs::directory_iterator(srcDir)) {
const fs::path& path = entry.path();
if (!SOURCE_FILE_EXTS.contains(path.extension())) {
continue;
}
if (!isLibSource(path)) {
continue;
}
if (libSource.empty()) {
libSource = path;
library = true;
} else {
throw PoacError("multiple lib sources were found");
}
Comment on lines +835 to +840
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (libSource.empty()) {
libSource = path;
library = true;
} else {
throw PoacError("multiple lib sources were found");
}
if (!libSource.empty()) {
throw PoacError("multiple lib sources were found");
}
libSource = path;
library = true;

}

if (!executable && !library) {
throw PoacError(fmt::format(
"neither src/main{} nor src/lib{} was not found", SOURCE_FILE_EXTS,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"neither src/main{} nor src/lib{} was not found", SOURCE_FILE_EXTS,
"src/(main|lib){} was not found",

SOURCE_FILE_EXTS
));
}

if (!fs::exists(outBasePath)) {
Expand All @@ -817,8 +853,16 @@ BuildConfig::configureBuild() {

setVariables();

std::unordered_set<std::string> allTargets = {};
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I now think just all for this variable name makes more sense. Thank you!

if (executable) {
allTargets.insert(packageName);
}
if (library) {
allTargets.insert(libName);
}

// Build rules
setAll({ packageName });
setAll(allTargets);
addPhony("all");

std::vector<fs::path> sourceFilePaths = listSourceFilePaths(srcDir);
Expand All @@ -832,7 +876,16 @@ BuildConfig::configureBuild() {
"Move it directly to 'src/' if intended as such.",
sourceFilePath.string()
);
} else if (sourceFilePath != libSource && isLibSource(sourceFilePath)) {
logger::warn(
"source file `{}` is named `lib` but is not located directly in the "
"`src/` directory. "
"This file will not be treated as a library. "
"Move it directly to 'src/' if intended as such.",
sourceFilePath.string()
);
}

srcs += ' ' + sourceFilePath.string();
}

Expand All @@ -842,16 +895,31 @@ BuildConfig::configureBuild() {
const std::unordered_set<std::string> buildObjTargets =
processSources(sourceFilePaths);

// Project binary target.
const std::string mainObjTarget = buildOutPath / "main.o";
std::unordered_set<std::string> projTargetDeps = { mainObjTarget };
collectBinDepObjs(
projTargetDeps, "",
targets.at(mainObjTarget).remDeps, // we don't need sourceFile
buildObjTargets
);
if (executable) {
// Project binary target.
const std::string mainObjTarget = buildOutPath / "main.o";
std::unordered_set<std::string> projTargetDeps = { mainObjTarget };
collectBinDepObjs(
projTargetDeps, "",
targets.at(mainObjTarget).remDeps, // we don't need sourceFile
buildObjTargets
);

Lazauya marked this conversation as resolved.
Show resolved Hide resolved
defineLinkTarget(outBasePath / packageName, projTargetDeps);
}

defineLinkTarget(outBasePath / packageName, projTargetDeps);
if (library) {
// Project library target.
const std::string libTarget = buildOutPath / "lib.o";
std::unordered_set<std::string> libTargetDeps = { libTarget };
collectBinDepObjs(
libTargetDeps, "",
targets.at(libTarget).remDeps, // we don't need sourceFile
buildObjTargets
);

defineLibTarget(outBasePath / libName, libTargetDeps);
}
Comment on lines +898 to +922
Copy link
Member

Choose a reason for hiding this comment

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

These are almost equivalent. Can you make a new method for this?


// Test Pass
std::unordered_set<std::string> testTargets;
Expand Down
21 changes: 21 additions & 0 deletions src/BuildConfig.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,16 @@ struct BuildConfig {

private:
std::string packageName;
std::string libName;
fs::path buildOutPath;
fs::path unittestOutPath;
bool isDebug;

// if we are building an executable
bool executable{ false };
Copy link
Member

Choose a reason for hiding this comment

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

Let's binary instead of executable as the term has already been used in the codebase. I think the comment for this variable is no longer needed, but I'd love to hear your thoughts.

Suggested change
bool executable{ false };
bool hasBinTarget{ false };

// if we are building a library
bool library{ false };
Copy link
Member

Choose a reason for hiding this comment

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

Likewise.


std::unordered_map<std::string, Variable> variables;
std::unordered_map<std::string, std::vector<std::string>> varDeps;
std::unordered_map<std::string, Target> targets;
Expand All @@ -67,6 +73,16 @@ struct BuildConfig {
public:
explicit BuildConfig(const std::string& packageName, bool isDebug = true);

bool isExecutable() const {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool isExecutable() const {
bool hasBinaryTarget() const {

return executable;
}
bool isLibrary() const {
Copy link
Member

Choose a reason for hiding this comment

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

Likewise.

return library;
}
ken-matsui marked this conversation as resolved.
Show resolved Hide resolved
std::string getLibName() const {
return this->libName;
}

void defineVar(
const std::string& name, const Variable& value,
const std::unordered_set<std::string>& dependsOn = {}
Expand All @@ -77,12 +93,14 @@ struct BuildConfig {
varDeps[dep].push_back(name);
}
}

void defineSimpleVar(
const std::string& name, const std::string& value,
const std::unordered_set<std::string>& dependsOn = {}
) {
defineVar(name, { .value = value, .type = VarType::Simple }, dependsOn);
}

void defineCondVar(
const std::string& name, const std::string& value,
const std::unordered_set<std::string>& dependsOn = {}
Expand Down Expand Up @@ -145,6 +163,9 @@ struct BuildConfig {
void defineLinkTarget(
const std::string& binTarget, const std::unordered_set<std::string>& deps
);
void defineLibTarget(
const std::string& libTarget, const std::unordered_set<std::string>& deps
);

void collectBinDepObjs( // NOLINT(misc-no-recursion)
std::unordered_set<std::string>& deps, std::string_view sourceFileName,
Expand Down
38 changes: 27 additions & 11 deletions src/Cmd/Build.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,29 +33,45 @@ const Subcmd BUILD_CMD =
.setMainFn(buildMain);

int
buildImpl(std::string& outDir, const bool isDebug) {
const auto start = std::chrono::steady_clock::now();

const BuildConfig config = emitMakefile(isDebug, /*includeDevDeps=*/false);
outDir = config.outBasePath;

const std::string& packageName = getPackageName();
runBuildCommand(
const std::string& outDir, const BuildConfig& config,
const std::string& targetName
) {
const Command makeCmd = getMakeCommand().addArg("-C").addArg(outDir).addArg(
(config.outBasePath / packageName).string()
(config.outBasePath / targetName).string()
);
Command checkUpToDateCmd = makeCmd;
checkUpToDateCmd.addArg("--question");

int exitCode = execCmd(checkUpToDateCmd);
if (exitCode != EXIT_SUCCESS) {
// If packageName binary is not up-to-date, compile it.
// If targetName binary is not up-to-date, compile it.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// If targetName binary is not up-to-date, compile it.
// If `targetName` Make target is not up-to-date, compile it.

logger::info(
"Compiling", "{} v{} ({})", packageName, getPackageVersion().toString(),
"Compiling", "{} v{} ({})", targetName, getPackageVersion().toString(),
getProjectBasePath().string()

);
exitCode = execCmd(makeCmd);
}
return exitCode;
}

int
buildImpl(std::string& outDir, const bool isDebug) {
const auto start = std::chrono::steady_clock::now();

const BuildConfig config = emitMakefile(isDebug, /*includeDevDeps=*/false);
outDir = config.outBasePath;

const std::string& packageName = getPackageName();
int exitCode = 0;
if (config.isExecutable()) {
exitCode = runBuildCommand(outDir, config, packageName);
}

if (config.isLibrary() && exitCode == 0) {
const std::string libName = config.getLibName();
exitCode = runBuildCommand(outDir, config, libName);
}

const auto end = std::chrono::steady_clock::now();
const std::chrono::duration<double> elapsed = end - start;
Expand Down