Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Tester develop #780

Closed
wants to merge 55 commits into from
Closed

Tester develop #780

wants to merge 55 commits into from

Conversation

swatanabe-b1
Copy link
Contributor

Change Description

Ports #674 to current eosio develop. Also works with release/2.0.x. Does not compile with release/1.8.x because of differences in chainlib.

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

Boost.Test only works in header only mode for now.  Building it as a
static library results in mysterious link errors.

Support stdio/iostreams in CDT (only works in the tester, not
in contracts as EOSIO has neither a filesystem nor stdin.)

Eliminate file handling and generic test library functionality from tester.
Instead of using std::variant, use a custom type that
serializes like variant, but directly exposes the latest
version of the type.  Only one type is supported for now.
Handling more than one type would require the ability to
convert from old formats.
Copy link
Contributor

@larryk85 larryk85 left a comment

Choose a reason for hiding this comment

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

Please remove the boost additions, for cdt we've been actively moving away from boost and I don't want to further the reliance on it. Second, you changed the header relative pathing, I would like that to go back to the way that it was. Relative paths allows CDT to block off automatic inclusion given different compilation contexts, second it ensures that only the header files we expect are the ones that are used.

@swatanabe-b1
Copy link
Contributor Author

I moved away from relative paths so that it could find the abieos headers.

@larryk85
Copy link
Contributor

larryk85 commented Mar 2, 2020

I moved away from relative paths so that it could find the abieos headers.

I would still like the relative includes. It saves a lot of headache if multiple places are in it's include path that could have the header files. Secondly, we should be installing the abieos headers in a specific location, that is in the CDT include path.

@swatanabe-b1
Copy link
Contributor Author

I moved away from relative paths so that it could find the abieos headers.

I would still like the relative includes. It saves a lot of headache if multiple places are in it's include path that could have the header files

It only saves headaches by papering over the underlying problem. When building CDT, we control the include paths, so it shouldn't be an issue. We also control the default include paths of the compiler, so it can only affect users who add their own includes. If they add an include path that conflicts, then it's a toss-up whether forcing use of our own headers will fix the problem or make things worse.

@larryk85
Copy link
Contributor

It’s not papering over a problem. It’s a library, it’s just good hygiene for one. Secondly, CDT doesn’t control things like eos installs. Which historically have installed to weird locations and has caused problems (so did cdt pre-1.2 (pre packages), I am not without blame in this too).

@swatanabe-b1
Copy link
Contributor Author

It’s not papering over a problem. It’s a library, it’s just good hygiene for one.

No it isn't. Your argument is somewhat valid for internal headers, but these headers are not internal. A library's public headers must have globally unique names that do not conflict with anything else in the user's include path. As long as this holds, <> includes are guaranteed to be safe. If this does not hold, then it isn't only CDT that's broken. Users' code is also directly broken.

Secondly, CDT doesn’t control things like eos installs. Which historically have installed to weird locations and has caused problems (so did cdt pre-1.2 (pre packages), I am not without blame in this too).

It shouldn't matter where eos is installed.

@larryk85
Copy link
Contributor

I don't think you get what I am stating. If I include in my code #include <eosio/variant.hpp, then in variant.hpp it has #include <eosio/foo.hpp> this can and has opened the door for weirdness. It's not the user's code that is the problem or CDT, it is installations of older projects, CDTs that are not ours, etc. Now, if variant.hpp is not in one of the other locations, then they are getting the header file they think but it is including other headers files that can have different semantics. I don't buy that it is either an internal vs. external argument. It is fundamentally just good practice to do so, it is more cumbersome for large projects. But, for small libraries there is no reason not too.

@swatanabe-b1
Copy link
Contributor Author

I understand exactly what you're saying and I disagree. The scenario that you've described is problematic regardless of how CDT handles its includes, because users can also #include <eosio/foo.hpp> and this will be equally broken. In fact, it will be even more broken, because it will cause two different versions of eosio/foo.hpp to be included in the same translation unit.

@jeffreyssmith2nd
Copy link
Contributor

Can this PR be closed since tester is now in eos? EOSIO/eos#9018

@swatanabe-b1
Copy link
Contributor Author

The wasm side is still in CDT, but this PR is really out-dated by now.

@jeffreyssmith2nd
Copy link
Contributor

jeffreyssmith2nd commented May 13, 2020

... this PR is really out-dated by now.

Agreed, I think it would be more valuable to open a new one with just the parts that we need in CDT.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants