Skip to content
This repository has been archived by the owner on Jan 19, 2024. It is now read-only.

default --jobs parameter to zero #45

Merged
merged 3 commits into from
Apr 28, 2023
Merged

default --jobs parameter to zero #45

merged 3 commits into from
Apr 28, 2023

Conversation

dimas1185
Copy link
Contributor

solved #28

@dimas1185 dimas1185 linked an issue Apr 21, 2023 that may be closed by this pull request
tools/build.hpp Outdated
@@ -18,7 +18,7 @@ namespace antler {
subcommand->footer(std::string(R"(Examples:)")
+ "\n\t" + app.get_name() +R"( build -j3)");
subcommand->add_option("-p, path", path, "This is the path to the root of the project.");
subcommand->add_option("-j, --jobs", jobs, "The number of submodules fetched at the same time.")->default_val(1);
subcommand->add_option("-j, --jobs", jobs, "The number of submodules fetched at the same time. Default is 0, which means to use git default.")->default_val(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe copy the text from git --help and specify what git will do when passed 0:

Suggested change
subcommand->add_option("-j, --jobs", jobs, "The number of submodules fetched at the same time. Default is 0, which means to use git default.")->default_val(0);
subcommand->add_option("-j, --jobs", jobs, "The number of submodules cloned in parallel. Default is 0, use cpu count.")->default_val(0);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like that because of if git going to ever change it we should do the same. Also I'm not sure this is same for all git versions so I would rather leave that to git. Your statement sounds like more specific but not 100% correct in all cases. my git says:
-j <n>, --jobs <n> The number of submodules fetched at the same time. Defaults to the submodule.fetchJobs option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point.But I'm not sure sending -j0 uses git's default either.

After more investigation, I don't think --jobs is used unless --recurse-submodules is also specified. We don't do that. I am less certain that this is the right answer for #28.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe let's just remove it at all? Any clue why it was added? It seems to me this is useless. At least in mvp.

Copy link
Contributor

Choose a reason for hiding this comment

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

No clue. I believe removal is appropriate. Our code will be simpler and more maintainable without jobs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I removed its usage for clone however it was also used for cmake build so I set it to processors number by default if specifying zero

Copy link
Contributor

@ScottBailey ScottBailey left a comment

Choose a reason for hiding this comment

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

See comment.

@ScottBailey
Copy link
Contributor

Based on this commit I do not believe sending --jobs=0 to git clone is a good idea.

Our target OSes do not contain the fixed release, 2.40.

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for adding docs!

@@ -97,7 +101,7 @@ namespace antler {

CLI::App* subcommand = nullptr;
std::string path;
uint32_t jobs = 1;
uint32_t jobs = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we switch this to a negative number then we don't NEED to pass it to cmake. I generally dislike when we lack an option to leave things alone.

Suggested change
uint32_t jobs = 0;
int32_t jobs = -1;

Copy link
Contributor

Choose a reason for hiding this comment

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

And line 43 could switch to

if(jobs < 0)
   return system::execute("cmake", {"--build", bin_dir.string()});
return system::execute("cmake", {"--build", bin_dir.string(), "-j", std::to_string(jobs)});

tools/build.hpp Outdated
@@ -18,7 +19,7 @@ namespace antler {
subcommand->footer(std::string(R"(Examples:)")
+ "\n\t" + app.get_name() +R"( build -j3)");
subcommand->add_option("-p, path", path, "This is the path to the root of the project.");
subcommand->add_option("-j, --jobs", jobs, "The number of submodules fetched at the same time.")->default_val(1);
subcommand->add_option("-j, --jobs", jobs, "The number of jobs to use with cmake. If 0 is specified (default value) antler will use number of available processors.")->default_val(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
subcommand->add_option("-j, --jobs", jobs, "The number of jobs to use with cmake. If 0 is specified (default value) antler will use number of available processors.")->default_val(0);
subcommand->add_option("-j, --jobs", jobs, "The number of jobs to use with cmake. If 0 is specified antler will use number of available processors.");

Copy link
Contributor

@ScottBailey ScottBailey left a comment

Choose a reason for hiding this comment

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

I would prefer that we not ALWAYS pass jobs flag. See comments. But I am willing to accept this as is.

@dimas1185 dimas1185 merged commit f9cf2b0 into main Apr 28, 2023
@dimas1185 dimas1185 deleted the jobs-number-fix branch April 28, 2023 03:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Bad default for jobs flag ignores user config
3 participants