Antler project changes #9

larryk85 · 2023-03-06T13:18:52Z

There is still more I have to do this. But getting the review started will be great and getting docs in line with this will also be good.

It is functioning, the external deps is still nerfed for right now, I will fix my branch and push those changes later today/tonight.

Quite a few changes have occurred.

You can play around with creating a project and smart contract and see how it works and test it for me. You can also pull the repo https://github.com/larryk85/antler-proj-hello-example for an example of a project that uses libraries and dependencies. It should fully propagate and build that project.

Apologies, I have more tests but they are in a few commits that I rolled back so that you can start the review today with something that you can use. Later today I will add those back along with throwing in the implementation to fully support the external dependencies.

jolly-fellow · 2023-03-07T13:18:32Z

src/object.cpp

@@ -108,40 +98,12 @@ 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 reutrn it.
+   // If possible, find a dependency with matching name and reurrn it.


jolly-fellow · 2023-03-07T14:36:53Z

You can play around with creating a project and smart contract and see how it works and test it for me. You can also pull the repo https://github.com/larryk85/antler-proj-hello-example for an example of a project that uses libraries and dependencies. It should fully propagate and build that project.

IMHO it should be https://github.com/larryk85/antler-proj-example-hello

dimas1185 · 2023-03-08T00:34:30Z

include/antler/project/object.hpp

-   /// @param opts  Compile time options for this object. May be empty.
-   object(type_t ot, std::string_view name, antler::project::language lang, std::string_view opts);
+   /// @param copts  Compile time options for this object. May be empty.
+   /// @param lopts  Compile time options for this object. May be empty.


Compile time options -> Linker options

dimas1185 · 2023-03-08T00:43:49Z

include/antler/project/object.hpp

-   type_t m_type = none;                               ///< Object type: app, lib, or test.
-   std::string m_name;                                 ///< Object name.
+   type_t m_type = type_t::error;                      ///< Object type: app, lib, or test.
+   std::string m_name = "";                            ///< Object name.


this looks like same as default construction. What is the reason for having that set explicitly?

No just missed that.

dimas1185 · 2023-03-08T01:03:47Z

include/antler/system/utils.hpp

+      std::size_t i=0, p=0;
+
+      for (; i < s.size(); i++) {
+         if (detail::is_delim<Delims...>(s[i])) {


we can get rid of lines 38-39 adding something like this:

if (detail::is_delim<Delims...>(s[i]) || i+1 == s.size()) { ... }

This will never pickup the last character of the final substring.

dimas1185 · 2023-03-08T01:25:53Z

src/project-parse.cpp

@@ -33,17 +37,18 @@ namespace { // anonymous
 /// @return An optional string that is populated with the file contents *if* the load was successful; otherwise, it's invalid for any error.
 [[nodiscard]] std::optional<std::string> load(const std::filesystem::path& path, std::ostream& os) {


I don't like we pass cerr stream as a class method parameter.
If we want to substitute entire cerr for unit tests purposes we can do it same way we have it in native library when we use _prints. So I recommend at least add TODO section about this for future

I have changed all of the error logging in another branch to fix some other issues that will be a follow on to this one.

dimas1185 · 2023-03-08T01:44:35Z

src/project-populate.cpp

+
+/// Test to see if a file has the magic maintenance string.
+/// @return true if the file either does NOT exist OR contains the magic
+[[nodiscard]] bool has_magic(const std::filesystem::path& path, std::ostream& error_stream = std::cerr) {


In the other branch to follow this PR all of the project-parse and project-populate has collapsed to just a handful of type overloads for yaml conversions. So, I can make any changes here, but they will be gone anyway.

Misspoke, project-print and parse have collapsed into a handful of conversion struct overloads.

Populate has transformed into a 'cmake' class that is handling all of the population so we can possibly change the 'backend' at a later date.

dimas1185 · 2023-03-08T01:50:11Z

src/project-print.cpp

+inline static c4::csubstr to_csubstr(std::string_view sv) noexcept { return {sv.data(), sv.size()}; }
+inline static c4::csubstr to_csubstr(token tok) noexcept { return to_csubstr(system::to_string(tok)); }
+inline static c4::csubstr to_csubstr(version v) noexcept { return to_csubstr(v.to_string()); }
+
 void project::print(std::ostream& os) const noexcept {


suggestion to remove stream passed as a parameter. At least add TODO section about that

dimas1185 · 2023-03-08T01:52:50Z

src/project.cpp

 //--- alphabetic --------------------------------------------------------------------------------------------------------
-


add TODO: remove error stream passed as a parameter in future in favor of other system (probably like we have innative library)

dimas1185 · 2023-03-08T01:58:11Z

tests/cmake_tests.cpp

+                                                        "add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/libs)\n"
+                                                        "add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/tests)\n\n";
+
+   REQUIRE( ss.str() == cmake_preamble_expected );


this test fails on my machine

antler-proj/tests/cmake_tests.cpp:30: FAILED:
REQUIRE( ss.str() == cmake_preamble_expected )
with expansion:
"# Generated with antler-proj tool, modify at your own risk
cmake_minimum_required(VERSION 3.11)
project("test_proj" VERSION 1.0.0)

"

"# Generated with antler-proj tool, modify at your own risk
cmake_minimum_required(VERSION 3.13)
project("test_proj" VERSION 1.0.0)
add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/apps)
add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/libs)
add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/tests)

"

All of these tests have been rewritten in the latest branch.

dimas1185 · 2023-03-08T02:08:08Z

include/antler/project/project.hpp

   /// @param error_stream  The stream to print failure reports to.
   /// @return true for success; false indidates failure.
-   [[nodiscard]] static bool init_dirs(const std::filesystem::path& path, bool expect_empty = true, std::ostream& error_stream = std::cerr) noexcept;
+   [[nodiscard]] static bool init_dirs(const std::filesystem::path& path, std::ostream& error_stream = std::cerr) noexcept;

   /// Search this and directories above for `project.yaml` file.
   /// @note if path extension is `.yaml` no directory search is performed, instead return value indicating existence of path a regular file.
   /// @param path  This is the search path to begin with; if the project file was found, it is updated to the path to that file.
   /// @return true if the project file was found and is a regular file; otherwise, false.
   [[nodiscard]] static bool update_path(std::filesystem::path& path) noexcept;


this interface seems to me too non-standard and confusing. I would redo this into something like this:

[[nodiscard]] static std::filesystem::path find_project(const std::filesystem::path& search_path) noexcept;

dimas1185 · 2023-03-08T02:09:37Z

tests/common.hpp

+inline bool remove_file(std::string_view fn) { return std::filesystem::remove_all(fn); }
+
+inline bool load_project(std::string_view fn, antler::project::project& proj) {
+   auto p = std::filesystem::canonical(std::filesystem::path(fn));


if that is needed, maybe we should move canonical conversion into update_path?

dimas1185 · 2023-03-08T02:13:44Z

include/antler/project/source.hpp

+   struct source <object::type_t::app> {
+      inline static void create_source_file(std::filesystem::path p, const object& obj) {
+         std::string name = std::string(obj.name());
+         p /= std::filesystem::path("apps") / name / (name+".cpp");


we are using filesystem::path a lot, I suggest to make a literal to reduce verbosity

std doesn't have a literal for path, we could add one. But, I would argue the major gain we would get here.

dimas1185 · 2023-03-08T02:15:27Z

tests/init_tests.cpp

+      REQUIRE(load_project("./foo", proj));
+
+      // should fail if directory exists
+      REQUIRE(!proj.init_dirs(std::filesystem::path("./foo")));


antler-proj/tests/init_tests.cpp:24: FAILED:
REQUIRE( !proj.init_dirs(std::filesystem::path("./foo")) )
with expansion:
false

dimas1185 · 2023-03-08T02:17:34Z

tests/string_from.cpp

@@ -1,4 +1,4 @@
-#include <test_common.hpp>


delete this test, string::from is unused

dimas1185 · 2023-03-08T02:25:30Z

tools/build.hpp

+         std::filesystem::create_directory(bin_dir);
+         std::string cmake_cmd = "cmake -S " + build_dir.string() + " -B " + bin_dir.string();
+
+         return system::execute("cmake -S " + build_dir.string() + " -B " + bin_dir.string()); //cmake_cmd);


remove debug comment

dimas1185 · 2023-03-08T02:29:44Z

tests/common.hpp

+
+inline bool remove_file(std::string_view fn) { return std::filesystem::remove_all(fn); }
+
+inline bool load_project(std::string_view fn, antler::project::project& proj) {


we have almost identical function in tools/common.hpp. likely it can be reused

dimas1185 · 2023-03-08T02:39:27Z

tools/main.cpp

+      : tup(depends<Ts>(app)...) {}
+
+   template <std::size_t I=0>
+   constexpr inline int exec() {


as per the code it calls exec from first tuple element that has subcommand pointer. Was this an intention?
if so it seems to me that we don't need it and can simply call antler::add_to_project.exec() directly.
If this supposed to call exec for every element with subcommand available, it should be redone.

subcommand in this case is a lexical interface that is only populated when the particular subcommand is given on the command line.

So, antler-proj add will create a subcommand so that elements subcommand will be true, if antler-proj remove is called then that's subcommand will be created and its exec will be called.

Its not designed to work with multiple subcommands being called at once, you can't do that via the command line system any how.

Also, by creating I mean that CLI11 will populate that subcommand and it overloads the bool conversion operator. So, true means the subcommand has been seen in the command string.

dimas1185 · 2023-03-08T02:48:28Z

tools/remove_from.hpp

+                  for (auto& o : objs) {
+                     if (o.remove_dependency(dep)) {
+                        std::cout << "Removing dependency: " << dep << " from: " << o.name() << std::endl;
+                     }


I think we should add else for tracking errors and perhaps move lambda out of this block so we can reuse it to replace lines 40-46

It isn't really an error here, it is a glob remove dependency. So it is perfectly valid to call with a dependency that the system doesn't even have. It would be a bit much to print that it didn't remove it from every other object in the system.

dimas1185

order of importance:

two tests failed on my machine: must be fixed
there are few comments for cleanup unused code and minor refactor that are nice to consider or add TODO for future. It should take ~30 minutes for those changes

dimas1185

based on daily call discussion, I'm approving this PR as an exception as there is another branch that supposed to overwrite it and replace tests

ScottBailey · 2023-03-13T14:51:16Z

based on daily call discussion, I'm approving this PR as an exception as there is another branch that supposed to overwrite it and replace tests

Same.

ScottBailey

Approving as a development change. Upcoming PRs will address problems here and there's little point in investing significant energy into this PR.

Fix clang-tidy warnings

Bucky Kittinger added 11 commits February 28, 2023 19:07

c++17 changes and starting changes to aproj binaries

9494963

some more fixes

4531e78

some more updates

ae4006c

added update for all types, fixed bugs, cleaned up location

575df71

restructuring, remove unneeded code, refactoring to resolve some bugs

20f2029

more fixups and cleanup

c627c4a

some more changes

d8da40a

adding cmake population, stub app and lib support

f04c718

added building

eddf50a

add lib support

ab35142

fixes for deps

9d9830a

larryk85 requested review from ScottBailey, jolly-fellow and dimas1185 March 6, 2023 13:18

ScottBailey added 3 commits March 6, 2023 13:58

Update to v2.13.9 for gcc10 and clang14

2c4df7f

Correct version constraint decode and renable additional testing.

f617a17

Correct version constraint decode and renable additional testing.

9bfb93f

jolly-fellow reviewed Mar 7, 2023

View reviewed changes

dimas1185 reviewed Mar 8, 2023

View reviewed changes

tests/string_from.cpp

@@ -1,4 +1,4 @@

#include <test_common.hpp>

Copy link

Contributor

dimas1185 Mar 8, 2023

Choose a reason for hiding this comment

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

delete this test, string::from is unused

dimas1185 reviewed Mar 8, 2023

View reviewed changes

dimas1185 suggested changes Mar 8, 2023

View reviewed changes

Fix clang-tidy warnings

866a8f2

dimas1185 approved these changes Mar 13, 2023

View reviewed changes

ScottBailey approved these changes Mar 13, 2023

View reviewed changes

Merge pull request #11 from AntelopeIO/mikelik/clang-tidy

16512c3

Fix clang-tidy warnings

ScottBailey merged commit fc772a7 into initial_dev Mar 13, 2023

ScottBailey deleted the larryk85/proj-changes branch March 13, 2023 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Antler project changes #9

Antler project changes #9

larryk85 commented Mar 6, 2023

jolly-fellow Mar 7, 2023

jolly-fellow commented Mar 7, 2023

dimas1185 Mar 8, 2023

larryk85 Mar 13, 2023

dimas1185 Mar 8, 2023

larryk85 Mar 13, 2023

dimas1185 Mar 8, 2023

larryk85 Mar 13, 2023

dimas1185 Mar 8, 2023 •

edited

Loading

larryk85 Mar 13, 2023

dimas1185 Mar 8, 2023

larryk85 Mar 13, 2023

larryk85 Mar 13, 2023

dimas1185 Mar 8, 2023

dimas1185 Mar 8, 2023

dimas1185 Mar 8, 2023

dimas1185 Mar 8, 2023 •

edited

Loading

larryk85 Mar 13, 2023

dimas1185 Mar 8, 2023

dimas1185 Mar 8, 2023

dimas1185 Mar 8, 2023

larryk85 Mar 13, 2023

dimas1185 Mar 8, 2023

dimas1185 Mar 8, 2023

dimas1185 Mar 8, 2023

dimas1185 Mar 8, 2023

dimas1185 Mar 8, 2023

larryk85 Mar 13, 2023 •

edited

Loading

dimas1185 Mar 8, 2023

larryk85 Mar 13, 2023

dimas1185 left a comment •

edited

Loading

dimas1185 left a comment

ScottBailey commented Mar 13, 2023

ScottBailey left a comment

		@@ -33,17 +37,18 @@ namespace { // anonymous
		/// @return An optional string that is populated with the file contents if the load was successful; otherwise, it's invalid for any error.
		[[nodiscard]] std::optional<std::string> load(const std::filesystem::path& path, std::ostream& os) {

		//--- alphabetic --------------------------------------------------------------------------------------------------------


		inline bool remove_file(std::string_view fn) { return std::filesystem::remove_all(fn); }

		inline bool load_project(std::string_view fn, antler::project::project& proj) {

Antler project changes #9

Antler project changes #9

Conversation

larryk85 commented Mar 6, 2023

Choose a reason for hiding this comment

jolly-fellow commented Mar 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas1185 Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas1185 Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

"

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larryk85 Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas1185 left a comment • edited Loading

Choose a reason for hiding this comment

dimas1185 left a comment

Choose a reason for hiding this comment

ScottBailey commented Mar 13, 2023

ScottBailey left a comment

Choose a reason for hiding this comment

dimas1185 Mar 8, 2023 •

edited

Loading

dimas1185 Mar 8, 2023 •

edited

Loading

larryk85 Mar 13, 2023 •

edited

Loading

dimas1185 left a comment •

edited

Loading