Compilation for 32-bit architectures on nightly

[rubdos]

Discussion points:

• Is a global nightly feature good in this case? Should I give it a better name?
• I didn't run the tests on an ARM 32-bit; would you want me to test that?
• pagecache::Lsn is now of type i64 when the nightly feature is enabled.

Let me know if you want me to change stuff! :-)

Issue #145 for reference.

[rubdos]

FYI; I ran a 32 bit ARM executable on my Moto Z Play (which admittedly supports should support aarch64):

running 3 tests
test tree::bound::test_bounds ... ok
test tree::prefix::test_prefix ... ok
test tree::prefix::test_prefix_cmp ... ok

[spacejam]

Sorry for being so slow with this! We can actually just use #[cfg(nightly)] and #[cfg(not(nightly))] instead of defining a new feature.

[spacejam]

The one other thing, is that maybe we want to generally use AtomicI64 when #[cfg(target_pointer_width = "32")], so we never even mention nightly or not, just try to pull in integer atomics and use them when target_pointer_width is 32. Then only define AtomicLsn as AtomicIsize when target_pointer_width = "64". What do you think about that approach?

[rubdos]

Sorry for being so slow with this! We can actually just use #[cfg(nightly)] and #[cfg(not(nightly))] instead of defining a new feature.

Didn't know that was a thing, will change.

The one other thing, is that maybe we want to generally use AtomicI64 when #[cfg(target_pointer_width = "32")], so we never even mention nightly or not, just try to pull in integer atomics and use them when target_pointer_width is 32. Then only define AtomicLsn as AtomicIsize when target_pointer_width = "64". What do you think about that approach?

That means that it will still give a strange error on 32 bit non-nightly. I'll have it throw some more meaningful message for that.

This also means that at a certain point in time, this construct will "just work" on stable, which means there'll be less incentive to change everything to i64, won't it? But I think I agree that this is a cleaner solution.

[rubdos]

This also means that at a certain point in time, this construct will "just work" on stable, which means there'll be less incentive to change everything to i64, won't it? But I think I agree that this is a cleaner solution.

It won't, I added this compile error:

error: 32 bit architectures require a nightly compiler for now.
               See https://github.com/spacejam/sled/issues/145
  --> crates/pagecache/src/lib.rs:9:1
   |
9  | / compile_error!("32 bit architectures require a nightly compiler for now.
10 | |                See https://github.com/spacejam/sled/issues/145");
   | |_________________________________________________________________^

error: aborting due to previous error

error: Could not compile `pagecache`.

To learn more, run the command again with --verbose.

[rubdos]

If you ack this one, let me squash it before you merge :-)

[rubdos]

Sorry for being so slow with this! We can actually just use #[cfg(nightly)] and #[cfg(not(nightly))] instead of defining a new feature.

Actually, I cannot seem to find any documentation beyond this thread on this, are you sure this exists? Doesn't seem to work here.

According to the peeps on #rust on Mozilla's IRC, there's no such thing yet! So I'll revert that for now :-)

[rubdos]

FYI: I just found out that std::sync::atomic::hint_core_should_pause() doesn't exist in nightly anymore, in iobuf.rs, line 1195, and the cfg(nightly) you have never hits the compiler. You can try it by having compile_error!("This gets compiled") in the second branch, with not(nightly), and then compile with a nightly compiler.

Probably needs a separate issue. :-)

[spacejam]

cfg(nightly) you have never hits the compiler.

my bad! that codepath is only triggered during tests, which I assumed were running some of in nightly, but that was a bad assumption. I think I got the idea from that thread you linked, and just tried it and it didn't blow up until you found it just now because I wasn't properly exercising that codepath.

[spacejam]

perhaps something from this travis file can be used to get i686 building on travis? https://github.com/japaric/cross/blob/master/.travis.yml

[rubdos]

perhaps something from this travis file can be used to get i686 building on travis? https://github.com/japaric/cross/blob/master/.travis.yml

Right, I didn't check whether the thing I pushed actually worked on travis >.<

Let me have a check :-)

[rubdos]

Hehe, that works. Would you like some more (32 bit) archs to be added?

Should I add a note to the README.md saying that 32 bit requires --features=nightly and a nightly compiler? :-)

[spacejam]

Should I add a note to the README.md saying that 32 bit requires --features=nightly and a nightly compiler? :-)

That would be wonderful! I'm really excited to have support for these architectures now :)

[rubdos]

Heh, you got yourself a race condition in the Travis build of the README change. Lol.

[spacejam]

@rubdos a gift from the test gods!

These failpoint property tests reveal a steady stream of error handling inconsistencies, it's great, but also feels like an endless stream of bugs sometimes, and can be a bit time consuming to fix. I believe they are relatively low priority compared to a couple other issues right now, so it might be a couple weeks before this is green for very long test runs, but most of the time it passes.

I'll rerun the test and merge your changes, and cut a new release with them :)

[spacejam]

@rubdos the latest versions of pagecache and sled have these changes in! thanks again for your good work on this :)

[rubdos]

Hooray! Thanks for the feedback! I am looking forward to the next patch! :D