Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

generate codegen testcode at buildtime #163

Merged
merged 2 commits into from
Jan 18, 2019
Merged

generate codegen testcode at buildtime #163

merged 2 commits into from
Jan 18, 2019

Conversation

zeenix
Copy link
Contributor

@zeenix zeenix commented Jan 17, 2019

Fixes #162

Zeeshan Ali added 2 commits January 17, 2019 16:59
codegen: Don't add deadcode attribute in generated code
6ffd639 
This breaks at least the case of generated code being imported through
`include!` macro:

```
error: an inner attribute is not permitted in this context
 --> /home/zeenix/checkout/dbus-rs/target/debug/build/codegen-tests-34abc2ff2e0eca43/out/policykit.rs:3:3
  |
3 | #![allow(dead_code)]
  |   ^
  |
  = note: inner attributes, like `#![no_std]`, annotate the item enclosing them, and are usually found at the beginning of source files. Outer attributes, like `#[test]`, annotate the item following them.
```

This will become a problem in the following patch, where we use `include!`
macro to include the generated code into the tests code.
codegen: Separate dummy crate for tests
a3b0b6e 
Let's make use of build script to generate the test code before the tests
are run but after dbus-codegen crate has been built.

Fixes diwic#162.
@diwic
Copy link
Owner

diwic commented Jan 17, 2019

Really nice, thanks for cleaning that up!

I was wondering if one could use the path attribute #[path = "foo.rs"] instead of include! in order to not have to remove the allow(dead_code) part? Or if that is not possible, write a file in OUT_DIR which can be included and just contains something like mod policykit.rs; pub use policykit::*;?
Because removing allow(dead_code) is going to be quite annoying I'm afraid...

@zeenix
Copy link
Contributor Author

zeenix commented Jan 17, 2019

Really nice, thanks for cleaning that up!

No worries. :)

I was wondering if one could use the path attribute #[path = "foo.rs"]

I'll have a look at both approaches you mentioned but what is the deal with allow(dead_code)? I didn't see any warnings about deadcode in cargo build/test output.

@zeenix
Copy link
Contributor Author

zeenix commented Jan 17, 2019

Also, we can have the allow(dead_code) in the including module instead and advertise that as the way to use generated code. If you recall I was wondering how I'll use generated code in my own (external) crate and seems this is a good way to do things and seems better than committing generated code in VCS.

@diwic
Copy link
Owner

diwic commented Jan 18, 2019

I'll have a look at both approaches you mentioned but what is the deal with allow(dead_code)? I didn't see any warnings about deadcode in cargo build/test output.

dbus-codegen, when run against some server, that server usually returns many interfaces, including Properties and Introspectable. We generate code for them, but it is seldom used by the program that's including the module. They end up being dead code.

Also, we can have the allow(dead_code) in the including module instead and advertise that as the way to use generated code.

We could, I would consider this the fallback option if neither of my proposed solutions above work.

@zeenix
Copy link
Contributor Author

zeenix commented Jan 18, 2019

dbus-codegen, when run against some server, that server usually returns many interfaces, including Properties and Introspectable. We generate code for them, but it is seldom used by the program that's including the module. They end up being dead code.

Yeah i understand that bit but as I said rustc didn't complain at all about dead code with my first patch only (i-e without my tests architecture changes). The reason is that there is already an #![allow(dead_code)] line above the mod generated line in the test1.rs and test2.rs. So I don't see the point of finding an alternative.

We could, I would consider this the fallback option if neither of my proposed solutions above work.

I looked into the path option you mentioned and the problem with that is that it expects the file in the same source directory if I understood the docs correctly: "the file path is relative to the directory the source file is located" https://doc.rust-lang.org/reference/items/modules.html

The second alternative you mentioned, sounds fine to me and I'll have a look at that, if I didn't convince you above. :)

@diwic diwic merged commit 1914fe0 into diwic:master Jan 18, 2019
@diwic
Copy link
Owner

diwic commented Jan 18, 2019

Ok so path will not work according to rust-lang/rust#48250

I decided it wasn't such a big deal to add "allow(dead_code)" when you use the module, and maybe we can just call it a feature that the resulting code can be used inside an include! macro.

@diwic
Copy link
Owner

diwic commented Jan 18, 2019

Ok, so after some more code review I realized it was a bit silly that we just generated the asref file without doing anything with it so I added a test for that as well. Apparently the "Generic" part isn't working yet...

@zeenix zeenix deleted the codegen-tests branch January 21, 2019 12:50
@zeenix
Copy link
Contributor Author

zeenix commented Jan 21, 2019

Ok, so after some more code review I realized it was a bit silly that we just generated the asref file without doing anything with it so I added a test for that as well. Apparently the "Generic" part isn't working yet...

Cool, things you find out when you add tests :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zeenix @diwic