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.

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

move examples/ to demo/ #75

Closed
wants to merge 3 commits into from
Closed

Conversation

tshepang
Copy link
Contributor

@tshepang tshepang commented May 2, 2020

Closes #72

@tshepang tshepang force-pushed the move-examples branch 3 times, most recently from 487950f to c1aee6f Compare May 2, 2020 23:02
@tshepang
Copy link
Contributor Author

tshepang commented May 2, 2020

Those failures have anything to do with my changes?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Those failures don't look related.

cc @svartalf @dignifiedquire Looks like openssl builds are failing on i686 and arm platforms again. Any idea what's up?

demos/Cargo.toml Show resolved Hide resolved
@svartalf
Copy link
Contributor

svartalf commented May 3, 2020

@stjepang Docker images which are used by cross by default do not have OpenSSL in them (see cross-rs/cross#229).

These changes are failing CI now because [[bin]] targets are compiled when cargo test (or cross test) called and that causes an OpenSSL linkage error; [[example]] targets are only checked right now and not really built during the CI execution:

https://github.com/stjepang/smol/blob/74b8276e999201836ff5e5f8515c6ec026c6c970/.github/workflows/build-and-test.yaml#L26-L30

One possible solution to solve this issue is to stop compiling [[bin]] targets for cross workflow job in here:

https://github.com/stjepang/smol/blob/74b8276e999201836ff5e5f8515c6ec026c6c970/.github/workflows/build-and-test.yaml#L65-L66

Yet, I'm personally not sure what are the benefits of whole this change, so it maybe worth just to leave everything as is. Also, I'm not a maintainer here, so it is just my two cents on a situation :)

@ghost
Copy link

ghost commented May 3, 2020

Yet, I'm personally not sure what are the benefits of whole this change, so it maybe worth just to leave everything as is.

Two reasons:

  1. Adding examples is hard. I can't just create examples/foo.rs and then run it. I need to first define it inside examples/Cargo.toml, which is a bit annoying. I often create temporary examples like that for local testing. I guess I could use src/bin/foo.rs instead but.. eh :)

  2. Running examples is not a smooth process because we need to cd into examples/. This might be surprising to new users, and requires users to look into the readme to figure out what's actually going on.

@svartalf
Copy link
Contributor

svartalf commented May 3, 2020

@stjepang thank you for clarification! It seems that [[bin]] sections should be removed from the demos/Cargo.toml file then?

Regarding the CI failure. Since there is no OpenSSL in the cross containers, it is possible to tweak CI a bit to make it work at least somehow:

  1. Stop compiling binary targets on cross test. Unfortunately, there is no smth like --exclude-target argument exist in cargo test target selection options, so all other targets should be set manually as in
cargo test --lib --tests --benches
  1. Since binary targets are not compiled now, at least we can "check" them, so additional job step should be added with
cargo check --bins

@ghost
Copy link

ghost commented May 3, 2020

It seems that [[bin]] sections should be removed from the demos/Cargo.toml file then?

I'm fine with [[bin]] sections there - those don't bother me because demos are not added very often.

Regarding the CI failure. Since there is no OpenSSL in the cross containers, it is possible to tweak CI a bit to make it work at least somehow:

Cool! That doesn't seem too bad.

@tshepang
Copy link
Contributor Author

tshepang commented May 3, 2020

It seems that [[bin]] sections should be removed from the demos/Cargo.toml file then?

@svartalf How would one run them if [[bin]] were removed?

@tshepang
Copy link
Contributor Author

tshepang commented May 4, 2020

@svartalf are you able to do those CI changes, because I don't actually understand.

@svartalf
Copy link
Contributor

svartalf commented May 4, 2020

I just found a better solution to this problem, which does not requires any CI modifications. So, here is a dependencies branch, which causes the compilation problem:

smol-demos v0.0.0 (/home/svartalf/projects/smol/demos)
├── async-native-tls v0.3.3
│   ├── native-tls v0.2.4
│   │   ├── openssl v0.10.29
│   │   │   └── openssl-sys v0.9.55

Fortunately to all of us, it is possible to enable vendored feature of openssl-sys through all these dependencies chain with help of the async-native-tls feature with the same name.
@tshepang can you apply the following patch and push the changes so we can check the CI results?

diff --git a/demos/Cargo.toml b/demos/Cargo.toml
index c15e8e3..7560ca3 100644
--- a/demos/Cargo.toml
+++ b/demos/Cargo.toml
@@ -16,7 +16,7 @@ timerfd = "1.1.1"
 [dependencies]
 anyhow = "1.0.28"
 async-h1 = "1.1.2"
-async-native-tls = "0.3.3"
+async-native-tls = { version = "0.3.3", features = ["vendored"] }
 async-std = "1.5.0"
 async-tungstenite = { version = "0.4.2", features = ["async-native-tls"] }
 base64 = "0.12.0"

@tshepang
Copy link
Contributor Author

tshepang commented May 5, 2020

That build failure happens because the examples/ demos/ are now checked in cross compile environments, but 2 of those failed (how does one access historical build results?). I just pushed a change that removes that (though that means one would have to navigate into demos/ to run them).

That build failure happens because the examples/ demos/ are now checked in cross compile environments, but 2 of those failed. I just pushed a change that removes that (though that means one would have to navigate into demos/ to run them).

@coveralls
Copy link

coveralls commented May 5, 2020

Pull Request Test Coverage Report for Build 95835664

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 29.994%

Totals Coverage Status
Change from base Build 94462064: 0%
Covered Lines: 102
Relevant Lines: 112

💛 - Coveralls

@tshepang
Copy link
Contributor Author

tshepang commented May 5, 2020

It seems that [[bin]] sections should be removed from the demos/Cargo.toml file then?

@svartalf How would one run them if [[bin]] were removed?

Maybe move the source code for bins to demos/src/bin, then one would no longer need to annotate Cargo.toml each time they add a new one.

@ahmed-masud
Copy link

Just a question?? why are you not just using examples as they are supposed to be used? Is it to keep dev-dependencies OUTSIDE of main Cargo.toml?

@tshepang
Copy link
Contributor Author

tshepang commented Jun 2, 2020

I think it was to keep CI builds fast, but am not sure.

@llebout
Copy link
Contributor

llebout commented Jul 12, 2020

@tshepang There was an issue with an example requiring the cssparser crate that failed to compile with some options required for code coverage. The main crate and examples were separated so that cssparser does not get pulled in even when we do not intend to do anything with the examples.

@tshepang
Copy link
Contributor Author

@leo-lb would it not be enough to add cssparser to [dev-dependencies]?

@llebout
Copy link
Contributor

llebout commented Jul 12, 2020

@tshepang I am not sure examples count as "dev"

See here for context: #13

@tshepang
Copy link
Contributor Author

@leo-lb just checked Cargo book...

Dev-dependencies are not used when compiling a package for building, but are used for compiling tests, examples, and benchmarks.

@llebout
Copy link
Contributor

llebout commented Jul 13, 2020

@tshepang We would need them to be used for examples but not tests, because tests are used for coverage, so it's not solving the issue.

@tshepang
Copy link
Contributor Author

this is made obsolete by #188

@tshepang tshepang closed this Jul 15, 2020
@tshepang tshepang deleted the move-examples branch July 15, 2020 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Rename the examples directory
5 participants