tz: add support for the Android platform #199
Conversation
*heavy sigh*
BurntSushi
force-pushed
the
ag/android
branch
4 times, most recently
from
c52e3cb to
404ce75
Compare
Android support has two prongs: * The special Android concatenated time zone database will now be read by Jiff automatically. * The `persist.sys.timezone` Android property is read to determine the system's configured IANA time zone identifier. Closes #140
BurntSushi
force-pushed
the
ag/android
branch
from
404ce75 to
1f0a7f7
Compare
| // SAFETY: `name` is a valid NUL terminated string and | ||
| // `system_property_find` is a valid function read from `dlsym` | ||
| // according to the declaration in `include/sys/system_properties.h`. |
There was a problem hiding this comment.
I believe this is meant to refer to system_property_read & prop_info being valid instead?
| # `integration` test target is being ignored. We don't include anything else | ||
| # so tests obviously won't work, but it makes `cargo package` quiet. |
There was a problem hiding this comment.
Would it be a problem for the Android build for some reason if we included the entire tests/ directory so tests work?
Context: I'm trying to import this crate in our repo at work and the tooling takes the files from the crate (not the Git commit) and tries to set up tests. I can easily configure it not to set up tests, so it's not a problem. I'm just wondering if there's a good reason for not including tests in the crate.
There was a problem hiding this comment.
The reason is that the test ls bloat the binary. And pretty much everyone wastes bandwidth on it. As I understand it, many crates do this, so I assume y'all already have proceses in place for dealing with this.
There was a problem hiding this comment.
The reason is that the test ls bloat the binary.
Do you mean the archive that's published on crates.io? Because I would assume that Rust binaries would not be bloated by tests in crates they depend on, right?
As I understand it, many crates do this, so I assume y'all already have proceses in place for dealing with this.
I've imported hundreds of crates (~250?) into Google's monorepo. It's not uncommon that tests fail for various reasons (e.g. attempting to read files that are not in the crate), but I don't think it's common that tests don't compile. I may just have forgotten some such cases, though.
Anyway, we do have processes to not even attempt to build the tests, so no problem.
There was a problem hiding this comment.
Sorry, yes, I mean the archive downloaded from crates.io.
I did this when I added a test tzdata file for Android. Even compressed, it adds an extra 100KB to the artifact. It wasn't obviously worth it to me to imclude it or spend the effort to make tests work in its absence. So I figured I would just remove everything.
I recently did this for bstr as well upon request. I'm considering doing it for my other crates as well.
How important is running tests from the crate downloaded for you? Do you commonly catch regressions?
There was a problem hiding this comment.
Thanks for explaining. That sounds reasonable.
How important is running tests from the crate downloaded for you? Do you commonly catch regressions?
I'm not on the Rust team, but I think it's not very important. As an indication, we typically don't add build targets for the tests if it requires importing additional dev dependencies.
I think I would not even have noticed if tests/lib.rs had not been in the archive. rust-lang/cargo#14290 seems to be the bug for getting a way to silence the warning and not have to include the test target.
There was a problem hiding this comment.
Sorry, just getting back around to this. I don't think I have anything else to add, but please let me know if this winds up being an issue. I'm open to revisiting it. Philosophically, I do like the idea of being able to run tests from the crate artifact.
Android support has two prongs:
by Jiff automatically.
persist.sys.timezoneAndroid property is read to determine thesystem's configured IANA time zone identifier.
Closes #140