EPEE Cleanup Reorganized #8211

jeffro256 · 2022-03-08T06:38:38Z

This PR is identical to #8175 but organized in a way that I believe is easier to verify for others. I will not adding any more of my own commits to this PR, but I'm ready to squash or add changes if/when this PR gets approved. If you want more granular comments on changes, I would refer to the old PR, but I'm happy to answer any questions. Thanks for your time!

mj-xmr · 2022-03-08T06:54:18Z

I would review this if the commits were separated into individual PRs. The reasons behind this are, that this reduces the risk of merge conflicts and that it's a lot of work to do in one go. For these 2 reasons you will typically not get a 2nd opinion, so in effect the PR will not be merged, which would lead to a lot of time waste from your and my side.

The 3rd reason for the split, that I personally have, is that some code can possibly still be useful in the future, even if it's unused for now. I'd like to have a possibility to express my opinion about it. The larger the PR, the harder it will be to reach a consensus on this, since the decision is basically binary (to merge or not to merge).

selsta · 2022-03-08T06:56:09Z

@mj-xmr they are separate here: #8175

but I think the problem was that 40 commits became too large

Edit: sorry I misread PRs and commits

selsta · 2022-03-08T07:02:48Z

@mj-xmr I initially said to keep it in one PR and I think it's fine if we discuss everything here, otherwise we will have 10 open PRs about epee cleanup. Most of these changes should be quite uncontroversial anyway.

mj-xmr · 2022-03-08T07:07:16Z

See? We have the 1st disagreement already :)

jeffro256 · 2022-03-08T07:15:39Z

I would review this if the commits were separated into individual PRs.

@mj-xmr What @selsta said about the old PR #8175 is correct. The old PR shows more of my thought process, has much finer changes, and more comments about each change. The problem is that it is lengthy and hard to digest. This PR I focused on ease of review.

The 3rd reason for the split, that I personally have, is that some code can possibly still be useful in the future, even if it's unused for now.

I considered this for sure, but I tend to fall under the camp of YAGNI. Many of the deletions are files/functions that were inherited from some botnet that predates the project itself and were never used. EPEE is very daunting for new developers and it doesn't help that ~25% of it is red herring code. Also I figured that the less cruft that you all have to deal with the better.

I'd like to have a possibility to express my opinion about it. The larger the PR, the harder it will be to reach a consensus on this, since the decision is basically binary (to merge or not to merge).

If you have an issue with a specific part of this PR I'd be happy to hear it, and I think I have it fairly well organized under the old PR, so I can rebase it if needed. Thanks for your time btw!

mj-xmr · 2022-03-08T07:25:57Z

@jeffro256 I will conform to how it was expected from you from @selsta , since this was the 1st suggestion in the row, while mine was only the 2nd one. Otherwise we'd be going in circles.

I'll have a look then. I just wish that the PR isn't left hanging after being reviewed.

jeffro256 · 2022-03-08T07:27:33Z

@selsta I think the vast majority of this PR should be uncontroversial but I'll try to point out a couple of things that might not be:

Refactoring get_ns_count() in time_helper.h (previously in misc_os_dependent.h) to use std::chrono::steady_clock instead of the medley of implementation-specific hacks it was using before
When factoring out pragma_comp_defs.h, I removed some macros which were suppressing warnings. I think I covered my bases, but there's a chance the change might generate a lot of spam on compilers that aren't GCC.
Moving copyable_atomic to be a subclass of cryptonote_connection_context (I commented on that change in the code)
Removing epee/tests since they weren't be used and would take a lot of refactoring to get to where they could compile

jeffro256 · 2022-03-08T07:43:22Z

@mj-xmr @selsta Another thing that I forgot to mention is that either PR should build with no issues no matter at which commit in the branch you pick. I hope that makes it a little easier :)

majestrate · 2022-03-08T12:20:30Z

this reduces the risk of merge conflicts

why would there merge conflicts in files untouched for 5+ years?

mj-xmr · 2022-03-09T15:39:16Z

why would there merge conflicts in files untouched for 5+ years?

I touched some of them myself not so long ago. Somebody else might be right now modifying some of them and starting a PR. But even if not, it's kind of a bad habit and it might bite back if continued this way. And after all, if in meantime somebody were to merge a change to just one of the files and create a conflict, this whole PR has to be updated, rather than a given smaller PR from the whole group. But anyway, I'll just get my hands dirty and start reviewing.

mj-xmr · 2022-03-09T15:41:09Z

@jeffro256

I considered this for sure, but I tend to fall under the camp of YAGNI. Many of the deletions are files/functions that were inherited from some botnet that predates the project itself and were never used. EPEE is very daunting for new developers and it doesn't help that ~25% of it is red herring code. Also I figured that the less cruft that you all have to deal with the better.

I agree. Especially since I saw, that with removal of some code from the templates, you took out some heavy boost headers along. I'm just saying that there still might be some nice code from the deletions that you did (like 5%?). Let me have a look and I'll just share my opinion based on my experience of what still could be needed regardless of YAGNI. These opinions can be also safely ignored.

contrib/epee/src/file_io_utils.cpp

mj-xmr · 2022-03-09T15:58:35Z

contrib/epee/src/file_io_utils.cpp

-	}
-
-
-	bool set_file_time(const std::string& path_to_file, const time_t& ft)


I think it could be useful and nicely wraps boost::filesystem's heavy headers.

On the other hand, boost::filesystem functions are already mentioned 245 times in 9 files throughout the project, and I believe boost already has functions for get_file_time and set_file_time: https://www.boost.org/doc/libs/1_58_0/libs/filesystem/doc/reference.html#last_write_time

You have to start from somewhere and lead by a good example, even if at the beginning it appears to make no sense, because it's already much worse somewhere else. Bit by bit the better solutions can purge the worse :)

contrib/epee/include/global_stream_operators.h

contrib/epee/include/ado_db_helper.h

contrib/epee/include/math_helper.h

contrib/epee/include/misc_language.h

mj-xmr · 2022-03-09T16:28:07Z

contrib/epee/include/misc_language.h

 #define AUTO_VAL_INIT(v)   boost::value_initialized<decltype(v)>()

 namespace misc_utils
 {
-	template<typename t_type>
-		t_type get_max_t_val(t_type t)


I'd leave it.

Why use get_max_t_val instead of std::numeric_limits<t_type>::max() directly? std::numeric_limits is already used 148 times throughout the project and IMO its confusing to pass in a raw value and receive a static limit unrelated to the value passed to the function.

As far as I see it evaluates to the same type and value.

It's not a state of the art function though anyway.

Yeah it should evaluate to to the same value, but it feels clunkier to use IMO because you pass a raw value of the type that you want to get the limit of, instead of just asking for it based on type.

contrib/epee/include/net/abstract_tcp_server.h

contrib/epee/include/time_helper.h

selsta · 2022-03-09T16:36:12Z

My personal opinion, I would remove everything unless it can be argued that a function is likely to be used in the future. Something just generally looking useful without any particular use in monero should be removed.

Ideally we would shrink epee over time with all new code getting added to src/.

contrib/epee/include/storages/portable_storage_to_bin.h

mj-xmr · 2022-03-09T16:49:47Z

contrib/epee/include/storages/gzipped_inmemstorage.h

-{
-
-	template<class t_base_storage>
-	class gziped_storage: public t_base_storage


Open for debate, but I find such a "demo" on how to use zipping important.

Should we create a doc then? Maybe we could put it in the EPEE Readme.

Sounds like a compromise for sure.

contrib/epee/include/time_helper.h

selsta · 2022-04-06T20:50:06Z

@jeffro256 They are separate patches so I would not squash. If we want to revert anything specific in the future it's easier this way.

mj-xmr

Rechecked.

mj-xmr · 2022-04-07T12:26:45Z

the dead code will still be in the git history if you need. just remove it so the development can speed up.

I agree.

How could there be any merge conflicts? ^_^

they can happen easily when contributors want to keep dead code around. this code existing in the current development branches really holds monero back, if you really want to see it you can always access it via an older tag. we removed these hunks and more in oxen ages ago and it allowed us to do magical things without being encumbered by tech debt. i can understand why no one has in monero, it's a painful tedious thankless task as no one is being paid full time to make the code tolerable and accessible.

the existence of merge conflicts on files used in dead code paths is an indicator of a deeper problem in the development and code review process.

Knowing ahead of time of the existence of the problem (plenty of dead code + one large PR, even though the commits are separate) and its accompanying risks (merge conflicts, preventing the merge of all of the commits, rather than only the affected ones), making a prediction, which got confirmed twice in both tries (merge bursts), yet still being confronted with criticism and disbelief(?) is a funny feeling.

BTW, I didn't insist on keeping the dead code around, just shared an opinion about a few specific functions from a pool of mud for others to consider.

Anyway, we're on the same page when it comes to highlighting the need of maintenance... in many forms.

you guys should invest in a project manager or a full time salaried senor lead developer.

The Monero's CCS is open for anybody. However I think, that Managers would not find this place very attractive for them. We need people who can do the actual work. Ability to manage is just a perk and is not reserved just to Managers.

What I think we should do is to simply learn from our own and each others' mistakes and carry on doing what we already do.

Currently working on an EPEE [ser/de]ialization library for Rust and at first glance, EPEE seemed to have support for optional wrappers. However, after looking into it, this feature appears to be half-baked and unused. Furthermore, adding support for optional values would be better suited to implement at the storage level, in my opinion. That would make parsing DOMs easier and less error-prone. If anyone is currently using this code, please comment. Thanks! At the time of writing, this PR has no merge conflicts with monero-project#8211

mj-xmr

Rechecked the recently synchronized file.

jeffro256 · 2022-04-18T14:16:42Z

@luigi1111 looks good!

Here lies dozens of unused files. This commit is ONLY file deletions except for the removing of a couple of #includes and removing filenames from CmakeLists where appropriate.

Remove unused include statements or unused definitions.

Actions: 1. Remove unused functions from misc_os_dependent.h 2. Move three remaining functions, get_gmt_time, get_ns_count, and get_tick_count into time_helper.h 3. Remove unused functions from time_helper.h 4. Refactor get_ns_count and get_internet_time_str and get_time_interval_string 5. Remove/add includes as needed Relevant commits on the old PR: a9fbe52 9a59b13 7fa9e28

Relevant commits on the old cleanup PR: 36933c7 21e43de 3c678bb

Relevant commit on old PR: 2499269

Relevant commit from old PR: bd0a511

Relevant commit from old PR: 330df29

Relevant commit in old PR: 1b798a7

@mj-xmr

At the request of @mj-xmr: monero-project#8211 (comment)

@mj-xmr

Thanks @mj-xmr: monero-project#8211 (comment)

jeffro256 · 2022-04-18T16:11:31Z

Rebased to clean up commit history @selsta

mj-xmr

Nothing changed from the reviewer's perspective.

Gingeropolous · 2022-04-18T18:31:39Z

Huzzah! Take that entropy!

jeffro256 mentioned this pull request Mar 8, 2022

Epee Cleanup #8175

Closed