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

Add support for locations starting with https://github.com/ #47

Merged
merged 4 commits into from
Apr 26, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
3 changes: 2 additions & 1 deletion include/antler/project/dependency.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <iostream>
#include <utility> // std::pair

#include "location.hpp"
#include "version.hpp"
#include "yaml.hpp"
#include "../system/utils.hpp"
Expand Down Expand Up @@ -60,7 +61,7 @@ class dependency {
[[nodiscard]] inline const std::string& location() const noexcept { return m_loc; }
/// Set the location field of this dependency.
/// @param s The new from location of this dependency.
void location(std::string s) noexcept { m_loc = std::move(s); }
void location(std::string s) noexcept { m_loc = location::normalize(std::move(s)); }

/// Report on the status of this dependencies from field: does it look like an archive?
/// @return true if location ends in an archive format (e.g. ".tar.gz", ".tgz", etc")
Expand Down
8 changes: 8 additions & 0 deletions include/antler/project/location.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,21 @@

namespace antler::project::location {

/// If location is github URL then strips out `https://github.com/` from URL
/// @param loc Github location - either full URL or shorthand
/// @return string_view shorthand version of full github URL
[[nodiscard]] std::string_view normalize(std::string_view loc);
mikelik marked this conversation as resolved.
Show resolved Hide resolved

/// @param l Location to evaluate.
/// @return true if l looks an archive.
[[nodiscard]] bool is_archive(std::string_view l);

/// @param l Location to evaluate.
/// @return true if l looks like a github repository.
[[nodiscard]] bool is_github_repo(std::string_view l);
/// @param github_url Full github URL with pointing to specific project
/// @return strips out `https://github.com/` from full URL
std::string_view strip_github_url_to_shorthand(std::string_view github_url);
/// @param l Location to evaluate.
/// @return true if l looks like an archive from github.
[[nodiscard]] bool is_github_archive(std::string_view l);
Expand Down
2 changes: 1 addition & 1 deletion include/antler/project/net_utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ namespace antler::project {

static inline bool is_shorthand(std::string_view s) {
auto sub = s.substr(0, s.find_last_of("/"));
return sub.size() != s.size() && sub.find_last_of("/") == std::string_view::npos;
return s.size() > (sub.size() + 1) && sub.find_last_of("/") == std::string_view::npos;
}

private:
Expand Down
6 changes: 5 additions & 1 deletion include/antler/project/object.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,11 @@ namespace antler::project {
const auto& itr = m_dependencies.find(dep.name());
bool has_value = itr == m_dependencies.end();
mikelik marked this conversation as resolved.
Show resolved Hide resolved
mikelik marked this conversation as resolved.
Show resolved Hide resolved

m_dependencies.emplace(dep.name(), std::move(dep));
if(!has_value)
itr->second = dep;
else
m_dependencies.emplace(dep.name(), std::move(dep));
dimas1185 marked this conversation as resolved.
Show resolved Hide resolved

return has_value;
}

Expand Down
1 change: 1 addition & 0 deletions include/antler/system/utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ namespace antler::system {
for (const auto& arg : args) {
cmd += " " + arg;
}
cmd += " 2>&1";
Copy link
Contributor

Choose a reason for hiding this comment

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

what does it affect? if there will be error you return nullopt and if not you'll unlikely to have error output at all. And even if you do, parsing of output will likely break.

Copy link
Member Author

Choose a reason for hiding this comment

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

It resolves issue: sh: 1: gh: not found in the original issue.

I believe Bucky's intention was to run command line gh --version quietly (the function name is execute_quiet).
While stdout was silenced, the stderr was still printing error (which was ignored anyway).

Copy link
Contributor

Choose a reason for hiding this comment

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

got you. in that case cmd += "2> /dev/null" sounds to me like more self-descriptive

Copy link
Member Author

Choose a reason for hiding this comment

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

In current usages of execute_quiet both 2>&1 and 2> /dev/null are fine.
If someone would like to read the stderr if the return code is 0 (there are such cases) with the existing solution he will have such chance. In yours we are losing stderr output. So I will leave the current solution.

Copy link
Contributor

@dimas1185 dimas1185 Apr 25, 2023

Choose a reason for hiding this comment

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

I doubt that could be a case. if we will have non-critical error output on zero return code that may corrupt parsing of a valid data. Ideally that should be similar to subprocess.run in python. i.e. stdout and stderr piping is customizable. but I would leave it to next issue if that will be really needed. If you believe your case is more likely to happen than mine, just leave comment like pipe error output from console to stdout so caller can parse error string and I'll approve

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done


FILE* h = popen(cmd.c_str(), "r");
if (h == nullptr) {
Expand Down
3 changes: 1 addition & 2 deletions src/dependency.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,11 @@ void dependency::patch_remove(const system::fs::path& path) noexcept {
m_patchfiles.erase(i);
}


void dependency::set(std::string nm, std::string_view loc, std::string_view tag, std::string_view rel, std::string_view hash) {

m_name = std::move(nm);

m_loc = loc;
m_loc = location::normalize(loc);

m_tag_or_commit = tag;
m_rel = rel;
Expand Down
23 changes: 20 additions & 3 deletions src/location.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,17 @@ bool is_archive(std::string_view s) {
ends_with(s, ".tar.zst");
}

static inline bool is_github(std::string_view s) { return starts_with(s, "https://github.com"); }
constexpr std::string_view github_com = "https://github.com/";

std::string_view normalize(std::string_view loc) {
if (is_github_repo(loc)) {
return strip_github_url_to_shorthand(loc);
mikelik marked this conversation as resolved.
Show resolved Hide resolved
}

return loc;
}

static inline bool is_github(std::string_view s) { return starts_with(s, github_com); }

bool is_github_archive(std::string_view s) { return is_github(s) && is_archive(s); }

Expand All @@ -32,14 +42,21 @@ bool is_github_shorthand(std::string_view s) { return github::is_shorthand(s); }

bool is_github_repo(std::string_view s) { return is_github(s) && !is_archive(s); }

std::string_view strip_github_url_to_shorthand(std::string_view github_url) {
if(github_url.size() <= github_com.size())
throw std::out_of_range("URL passed: "+ std::string(github_url) + " should have minimum size of " + std::to_string(github_com.size() + 1));
mikelik marked this conversation as resolved.
Show resolved Hide resolved

return github_url.substr(github_com.size());
}

bool is_reachable(std::string_view l) {
if (!is_github_shorthand(l)) {
system::error_log("In this version of antler-proj only github shorthands (i.e. org/project) are supported. Generalized git repos and archives will be supported in a future version.");
system::error_log("In this version of antler-proj only github shorthands (i.e. org/project) and github URLs (i.e. https://github.com/org/project) are supported. Archives will be supported in a future version.");
return false;
}

return github{}.is_reachable(l);
// TODO add support for general git repos and archives
// TODO add support for archives
if (is_github_repo(l) || is_github_shorthand(l)) {
return github{}.is_reachable(l);
} else if (is_archive(l) || is_url(l) || is_github_archive(l)) {
Expand Down
2 changes: 1 addition & 1 deletion src/object.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ object::type_t object::type() const noexcept {


void object::upsert_dependency(antler::project::dependency&& dep) noexcept {
// If possible, find a dependency with matching name and reurrn it.
// If possible, find a dependency with matching name and return it.
auto i = std::find_if(m_dependencies.begin(), m_dependencies.end(), [dep](const antler::project::dependency& d) { return d.name() == dep.name(); } );
if( i == m_dependencies.end() )
m_dependencies.emplace_back(dep);
Expand Down
5 changes: 3 additions & 2 deletions src/project-parse.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,12 @@ template <typename NODE_T>
os << "Duplicate " << tok << " values in dependency list: " << i.val() << ", " << rv.location() << "\n";
return {};
}
if (!dependency::validate_location(sv_from_csubstr(i.val()))) {
auto location = normalize(sv_from_csubstr(i.val()));
if (!dependency::validate_location(location)) {
os << "Invalid location: " << i.val() << "\n";
return {};
}
rv.location(sv_from_csubstr(i.val()));
rv.location(location);
} break;

case token::patch: {
Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ add_executable(unit_tests
dependency_tests.cpp
location_tests.cpp
object_tests.cpp
net_utils_tests.cpp
project_tests.cpp
init_tests.cpp
version_constraint_tests.cpp
Expand Down
7 changes: 0 additions & 7 deletions tests/dependency_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,6 @@ TEST_CASE("Testing dependency validating locations") {
CHECK(d.location() == "larryk85/does-not-exist");

CHECK(!d.is_valid_location());

// TODO: reinstate these tests when support for general git repos and archives is added
//d.location("https://github.com/larryk85/cturtle");
dimas1185 marked this conversation as resolved.
Show resolved Hide resolved
//CHECK(d.is_valid_location());

//d.location("https://github.com/larryk85/does-not-exist");
//CHECK(!d.is_valid_location());
}

TEST_CASE("Testing dependency yaml conversions") {
Expand Down
28 changes: 24 additions & 4 deletions tests/location_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@

#include <catch2/catch.hpp>

TEST_CASE("Testing location clone") {
using namespace antler::project;
using namespace std::literals;
using namespace antler::project;

TEST_CASE("Testing location clone") {
antler::system::fs::remove_all("./clone_test");

CHECK(location::clone_github_repo("antelopeio", "antler-proj", "main", 10, "./clone_test/foo2"));
Expand All @@ -17,8 +18,6 @@ TEST_CASE("Testing location clone") {
}

TEST_CASE("Testing location github REST API requests") {
using namespace antler::project;

std::string default_branch = location::get_github_default_branch("antelopeio", "antler-proj");
CHECK(default_branch == "main");

Expand All @@ -27,3 +26,24 @@ TEST_CASE("Testing location github REST API requests") {

CHECK_THROWS(location::get_github_default_branch("antelopeio", "repo-does-not-exist"));
}

TEST_CASE("strip_github_url_to_shorthand") {
CHECK(location::strip_github_url_to_shorthand("https://github.com/org/project"sv) == "org/project"sv);
CHECK(location::strip_github_url_to_shorthand("https://github.com/org/project/z"sv) == "org/project/z"sv);
CHECK(location::strip_github_url_to_shorthand("https://github.com/org"sv) == "org"sv);
CHECK_THROWS(location::strip_github_url_to_shorthand("https://github.com"sv));
CHECK_THROWS(location::strip_github_url_to_shorthand("https://github.com/"sv));
CHECK_THROWS(location::strip_github_url_to_shorthand(""sv));
}

TEST_CASE("normalize") {
CHECK(location::normalize("https://github.com/org/project"sv) == "org/project"sv);
CHECK(location::normalize("https://github.com/org/project/z"sv) == "org/project/z"sv);
CHECK(location::normalize("https://github.com/org"sv) == "org"sv);
CHECK(location::normalize("https://xyz.com"sv) == "https://xyz.com"sv);
CHECK(location::normalize(""sv) == ""sv);
CHECK(location::normalize("https://github.com"sv) == "https://github.com"sv);
CHECK(location::normalize("https://github.saobby.my.eu.orgx"sv) == "https://github.saobby.my.eu.orgx"sv);

CHECK_THROWS(location::normalize("https://github.com/"sv));
}
20 changes: 20 additions & 0 deletions tests/net_utils_tests.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/// @copyright See `LICENSE` in the root directory of this project.

#include <antler/project/net_utils.hpp>

#include <catch2/catch.hpp>

using namespace std::literals;

TEST_CASE("shorthand") {
using namespace antler::project;

CHECK(github::is_shorthand("org/project"sv));
CHECK(github::is_shorthand("org/a"sv));

CHECK(github::is_shorthand("https://github.com/org/project"sv) == false);
CHECK(github::is_shorthand("org/project/"sv) == false);
CHECK(github::is_shorthand("org/"sv) == false);
CHECK(github::is_shorthand("org"sv) == false);
CHECK(github::is_shorthand(""sv) == false);
}
8 changes: 7 additions & 1 deletion tests/project_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ TEST_CASE("Testing project") {
{"appd", "C++", "", ""} };

apps[0].upsert_dependency({"foo", "https://github.com/larryk85/dune", "v13.3"});
apps[0].upsert_dependency({"foo", "mikelik/dune", "v13.4"});
apps[1].upsert_dependency({"bar", "https://github.com/larryk85/fast_math", "blah"});
apps[0].upsert_dependency({"baz", "https://github.com/antelopeio/leap", "v2.2.2v"});
apps[1].upsert_dependency({"libc", ""});
Expand All @@ -22,7 +23,8 @@ TEST_CASE("Testing project") {
{"libc", "C", "", ""},
{"libd", "C++", "", ""} };

libs[0].upsert_dependency({"foo", "https://github.com/larryk85/dune", "v13.3"});
libs[0].upsert_dependency({"foo", "https://github.com/larryk85/dune", "main"});
libs[0].upsert_dependency({"foo", "mikelik/dune2", "branch"});
libs[0].upsert_dependency({"bar", "https://github.com/larryk85/fast_math", "blah"});
libs[1].upsert_dependency({"baz", "https://github.com/antelopeio/leap", "v2.2.2v"});

Expand All @@ -43,7 +45,11 @@ TEST_CASE("Testing project") {
CHECK(proj.version().to_string() == "1.3.4");

CHECK(proj.apps().size() == 4);
CHECK(proj.apps()["appa"].dependencies()["foo"].location() == "mikelik/dune");
CHECK(proj.apps()["appa"].dependencies()["foo"].tag() == "v13.4");
CHECK(proj.libs().size() == 3);
CHECK(proj.libs()["libb"].dependencies()["foo"].location() == "mikelik/dune2");
CHECK(proj.libs()["libb"].dependencies()["foo"].tag() == "branch");
}


Expand Down
2 changes: 1 addition & 1 deletion tools/add_to.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ namespace antler {
obj.upsert_dependency(std::move(dep));

// We have values, so query the user if they want to apply.
system::info_log("\nObject name (to update): {0}\n"
system::info_log("Adding dependency:\nObject name (to update): {0}\n"
"Dependency name: {1}\n"
"Dependency location: {2}\n"
"tag/commit hash: {3}\n"
Expand Down