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

NO_STD #370

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

NO_STD #370

wants to merge 4 commits into from

Conversation

zendurix
Copy link

I have succesfully implemented no_std with hasbrown crate (to enable no_std use with default-features = false
It passed no_std check (https://github.com/mystor/cargo-no-std-check)

Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

I am not sure use crate::lib::*; is a good practice. The idea to wrap everything is good, but probably better to import only used modules. Please also add the corresponding CI job from #238

src/lib.rs Outdated Show resolved Hide resolved
@XVilka
Copy link
Member

XVilka commented Aug 26, 2020

Also CI is failing:

error[E0432]: unresolved imports `rand::thread_rng`, `rand::ChaChaRng`
 --> tests/unionfind.rs:5:12
  |
5 | use rand::{thread_rng, ChaChaRng, Rng, SeedableRng};
  |            ^^^^^^^^^^  ^^^^^^^^^ no `ChaChaRng` in the root
  |            |
  |            no `thread_rng` in the root
warning: unused import: `SeedableRng`
 --> tests/unionfind.rs:5:40
  |
5 | use rand::{thread_rng, ChaChaRng, Rng, SeedableRng};
  |                                        ^^^^^^^^^^^
  |
  = note: #[warn(unused_imports)] on by default
warning: unused import: `Rng`
 --> tests/unionfind.rs:5:35
  |
5 | use rand::{thread_rng, ChaChaRng, Rng, SeedableRng};
  |                                   ^^^
error: aborting due to previous error
For more information about this error, try `rustc --explain E0432`.
error: Could not compile `petgraph`.

@zendurix
Copy link
Author

Should all test be included in no_std? this will require some additional work

@XVilka
Copy link
Member

XVilka commented Aug 26, 2020

Yes.

@zendurix
Copy link
Author

zendurix commented Aug 26, 2020

Maybe leave thoose tests that uses RNG outside of no_std?

@XVilka
Copy link
Member

XVilka commented Aug 26, 2020

Yes, that makes sense.

@zendurix
Copy link
Author

I have implemented requested changes, all tests are passing in both std and no_std

@XVilka
Copy link
Member

XVilka commented Aug 26, 2020

Looks better, thanks!
Could you please also add a no_std CI job?

@zendurix
Copy link
Author

@XVilka
I am not sure how to do that, should I just add this line at the end of travis.yml ?
&& cargo test --all --no-default-features --features="graphmap","stable_graph","matrix_graph"

@XVilka
Copy link
Member

XVilka commented Aug 26, 2020

@zendurix you can just take .travis.yml from the old PR: https://github.com/petgraph/petgraph/pull/238/files#diff-354f30a63fb0907d4ad57269548329e3

@XVilka
Copy link
Member

XVilka commented Aug 26, 2020

Also I am not sure, instead of adding #[cfg(feature="std")] before every println! maybe it's better to define own macro or function and do the switch once?

By the way, you can also use https://github.com/hobofan/cargo-nono for checks.

@zendurix
Copy link
Author

Also I am not sure, instead of adding #[cfg(feature="std")] before every println! maybe it's better to define own macro or function and do the switch once?

By the way, you can also use https://github.com/hobofan/cargo-nono for checks.

I can think of smth like

pub fn print(args) {
   #[cfg(feature = "std)]
   println!(args)
}

But I am not very good with macros, and I am not sure how to do this

@XVilka
Copy link
Member

XVilka commented Aug 26, 2020

Maybe something like

macro_rules! no_std_println {
  ($(t:tt)*) => {
    #[cfg(feature = "std")]
    {
      println!($($t)*);
    }
  };
}

@zendurix
Copy link
Author

I see problems with CI (cause old PR have feature no_std) I will impl this macro and fix CI later today

@XVilka
Copy link
Member

XVilka commented Jan 13, 2021

@zendurix have you had any luck with this update? Could you please rebase because we migrated from Travis CI to GitHub Actions?

@XVilka
Copy link
Member

XVilka commented Mar 24, 2021

@zendurix could you please rebase your PR?

#[cfg(feature = "std")]
pub use std::collections::{hash_map::Entry::{Occupied, Vacant}, HashSet, hash_map::Iter};
#[cfg(not(feature = "std"))]
pub use hashbrown::{ HashSet, hash_map::{ HashMap, Entry::{Occupied, Vacant}, Iter}};
Copy link
Member

@bluss bluss May 16, 2021

Choose a reason for hiding this comment

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

HashMap is used in the public API - this PR would mean that public API changes breakingly based on a cargo feature, and that's not an allowed usage of cargo features in general. This can't be integrated with this design, unfortunately.

The whole approach to HashMap needs to change somehow. I don't know if using hashbrown is necessary. Keep it simple and incremental, is it possible to support compiling without HashMap?

@chaosprint
Copy link

Hi, this is such a wonderful job. Is there any plan that the rebase can be done in the near future?

@indietyp indietyp added the 0.7.0-fixed Issue fixed in 0.7.0 label Oct 23, 2023
@indietyp
Copy link
Member

0.7.0 will have no-std support, until this version has landed I will keep this PR open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.7.0-fixed Issue fixed in 0.7.0 rebase-needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants