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

fix: Build under MacOS #4

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: Build under MacOS #4

wants to merge 6 commits into from

Conversation

ovr
Copy link
Contributor

@ovr ovr commented May 6, 2024

Hello!

I've fixed an issue with compilation on MacOS and added CI.
Some tests fail with an issue that uniq on MacOS doesn't support -w1, I don't know how to port it.

Fix ##3

Thanks

@vlad17
Copy link
Owner

vlad17 commented May 6, 2024

Hi! Thank you for taking a look! Which tests fail due to the uniq issue? Can you post more information about the failure?

@ovr ovr mentioned this pull request May 6, 2024
.github/workflows/push.yml Outdated Show resolved Hide resolved
@ovr
Copy link
Contributor Author

ovr commented May 6, 2024

@vlad17

❯ cargo build
   Compiling dsrs v0.6.1 (/Users/ovr/projects/cube/datasketches-rs)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.27s
❯ cargo test
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.09s
     Running unittests src/lib.rs (target/debug/deps/dsrs-11c3d158f6d373f0)

running 23 tests
test wrapper::cpc::tests::union_empty ... ok
test wrapper::cpc::tests::cpc_empty ... ok
test wrapper::hh::tests::basic_merge ... ok
test wrapper::hh::tests::basic_heavy ... ok
test wrapper::hh::tests::check_hh_lgk4_multiplier2_nunique2 ... ok
test wrapper::hh::tests::check_hh_lgk4_multiplier20_nunique2 ... ok
test wrapper::hh::tests::check_hh_lgk4_multiplier20_nunique1 ... ok
test wrapper::hh::tests::check_hh_lgk4_multiplier2_nunique1 ... ok
test wrapper::hh::tests::check_hh_lgk4_multiplier2_nunique3 ... ok
test wrapper::hh::tests::hh_empty ... ok
test wrapper::hh::tests::retains_all_clone ... ok
test wrapper::hh::tests::retains_all ... ok
test wrapper::hh::tests::check_hh_lgk4_multiplier20_nunique3 ... ok
test wrapper::hh::tests::check_hh_lgk4_multiplier5_nunique1 ... ok
test wrapper::hh::tests::check_hh_lgk4_multiplier5_nunique3 ... ok
test wrapper::hh::tests::check_hh_lgk4_multiplier5_nunique2 ... ok
test stream_reducer::tests::reduces_stream ... ok
test wrapper::cpc::tests::basic_union_distinct ... ok
test wrapper::cpc::tests::basic_count_distinct ... ok
test wrapper::cpc::tests::basic_union_overlap ... ok
test wrapper::theta::tests::basic_count_distinct ... ok
test wrapper::theta::tests::basic_intersect_overlap ... ok
test wrapper::theta::tests::basic_intersect ... ok

test result: ok. 23 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.05s

     Running unittests src/main.rs (target/debug/deps/dsrs-c04cf0ba333b261d)

running 10 tests
test tests::keyed_count_empty ... FAILED
test tests::hh_unique_lines ... ok
test tests::unique_lines ... FAILED
test tests::count_empty ... FAILED
test tests::equally_dup_lines ... FAILED
test tests::hh_count_empty ... FAILED
test tests::hh_equally_dup_lines ... FAILED
test tests::unequally_dup_lines ... FAILED
test tests::unique_keyed_lines ... FAILED
test tests::keyed_dup_lines ... FAILED

failures:

---- tests::keyed_count_empty stdout ----
thread 'tests::keyed_count_empty' panicked at src/main.rs:236:9:
uniq: invalid option -- w
usage: uniq [-c | -d | -D | -u] [-i] [-f fields] [-s chars] [input [output]]

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tests::unique_lines stdout ----
thread 'tests::unique_lines' panicked at src/main.rs:385:9:
assertion `left == right` failed: 
unix:
     100

dsrs:
100

  left: [32, 32, 32, 32, 32, 49, 48, 48, 10]
 right: [49, 48, 48, 10]

---- tests::count_empty stdout ----
thread 'tests::count_empty' panicked at src/main.rs:385:9:
assertion `left == right` failed: 
unix:
       2

dsrs:
2

  left: [32, 32, 32, 32, 32, 32, 32, 50, 10]
 right: [50, 10]

---- tests::equally_dup_lines stdout ----
thread 'tests::equally_dup_lines' panicked at src/main.rs:385:9:
assertion `left == right` failed: 
unix:
     100

dsrs:
100

  left: [32, 32, 32, 32, 32, 49, 48, 48, 10]
 right: [49, 48, 48, 10]

---- tests::hh_count_empty stdout ----
thread 'tests::hh_count_empty' panicked at src/main.rs:385:9:
assertion `left == right` failed: 
unix:
1 1

dsrs:
2 

  left: [49, 32, 49, 10]
 right: [50, 32, 10]

---- tests::hh_equally_dup_lines stdout ----
thread 'tests::hh_equally_dup_lines' panicked at src/main.rs:385:9:
assertion `left == right` failed: 
unix:
1 1000
1001 2
1001 3

dsrs:
1001 1
1001 2
1001 3

  left: [49, 32, 49, 48, 48, 48, 10, 49, 48, 48, 49, 32, 50, 10, 49, 48, 48, 49, 32, 51, 10]
 right: [49, 48, 48, 49, 32, 49, 10, 49, 48, 48, 49, 32, 50, 10, 49, 48, 48, 49, 32, 51, 10]

---- tests::unequally_dup_lines stdout ----
thread 'tests::unequally_dup_lines' panicked at src/main.rs:385:9:
assertion `left == right` failed: 
unix:
     100

dsrs:
100

  left: [32, 32, 32, 32, 32, 49, 48, 48, 10]
 right: [49, 48, 48, 10]

---- tests::unique_keyed_lines stdout ----
thread 'tests::unique_keyed_lines' panicked at src/main.rs:236:9:
uniq: invalid option -- w
usage: uniq [-c | -d | -D | -u] [-i] [-f fields] [-s chars] [input [output]]


---- tests::keyed_dup_lines stdout ----
thread 'tests::keyed_dup_lines' panicked at src/main.rs:236:9:
uniq: invalid option -- w
usage: uniq [-c | -d | -D | -u] [-i] [-f fields] [-s chars] [input [output]]



failures:
    tests::count_empty
    tests::equally_dup_lines
    tests::hh_count_empty
    tests::hh_equally_dup_lines
    tests::keyed_count_empty
    tests::keyed_dup_lines
    tests::unequally_dup_lines
    tests::unique_keyed_lines
    tests::unique_lines

@vlad17
Copy link
Owner

vlad17 commented May 6, 2024

It looks like Mac doesn't work since it doesn't have -w1 like you said. But it should have -f1 (skip the first field when checking).

So instead of

sort --unique | uniq -w1 -c | awk '{print$2\" \"$1}'"

let's instead use

sort --unique | awk '{print$2\" \"$1}'" | uniq -f1 -c 

or something similar (so that formatting matches).

For keyed_count_empty, can we just hardcode the expected response?

@timbray
Copy link

timbray commented May 6, 2024

Doesn't seem to work?

cube/datasketches-rs> git remote -v
origin	[email protected]:cube-js/datasketches-rs.git (fetch)
origin	[email protected]:cube-js/datasketches-rs.git (push)
cube/datasketches-rs> git branch
* fix/macos-build
  main
cube/datasketches-rs> cargo build
error: process didn't exit successfully: `rustc -vV` (signal: 6, SIGABRT: process abort signal)
--- stderr
dyld[77711]: Symbol not found: __ZN4llvm10PGOOptionsC1ENSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEES7_S7_S7_NS_18IntrusiveRefCntPtrINS_3vfs10FileSystemEEENS0_9PGOActionENS0_11CSPGOActionEbb
  Referenced from: <EEB49284-2EAD-3371-9401-139E8557678B> /opt/homebrew/Cellar/rust/1.77.2_1/lib/librustc_driver-3dc17bded25d4e59.dylib
  Expected in:     <0C3379D6-AFA4-3FF3-97DD-387FECFCA3CF> /opt/homebrew/Cellar/llvm/18.1.5/lib/libLLVM.dylib

@ovr
Copy link
Contributor Author

ovr commented May 6, 2024

@timbray Could you try with https://rustup.rs/? llvm is a part of rust; as I can see from the error, you have a different llvm other than rust uses.

@timbray
Copy link

timbray commented May 6, 2024

Oops, sorry, my fault. Something went wrong and an llvm dependency was missing. cargo build works now but cargo test fails, here are the last few lines:



failures:
    tests::count_empty
    tests::equally_dup_lines
    tests::keyed_count_empty
    tests::keyed_dup_lines
    tests::unequally_dup_lines
    tests::unique_keyed_lines
    tests::unique_lines

test result: FAILED. 3 passed; 7 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.01s

error: test failed, to rerun pass `--bin dsrs`

@ovr
Copy link
Contributor Author

ovr commented May 6, 2024

@vlad17, could you please approve the CI run? It's strange, but I don't see CI status.

@ovr
Copy link
Contributor Author

ovr commented May 6, 2024

I've tested changes in my fork, it works https://github.com/cube-js/datasketches-rs/actions/runs/8976208069/job/24652549213

@vlad17
Copy link
Owner

vlad17 commented May 6, 2024

@ovr I approved the PR; but given the yml isn't master yet it may not have CI on.

@timbray as of @ovr 's recent changes tests should pass now

@ovr
Copy link
Contributor Author

ovr commented May 12, 2024

@vlad17, any plans to merge? Thanks

@vlad17
Copy link
Owner

vlad17 commented May 12, 2024

Thanks for the bump, sorry about the delay. I was waiting for @timbray to comment if tests pass for his MacOs setup as of the new changes.

Tim, do things work for you now with this branch?

@timbray
Copy link

timbray commented May 13, 2024

I'm having trouble figuring out how to get the right branch. (I'm a bit of a git moron). It looks like I should clone [email protected]:cube-js/datasketches-rs.git which builds but fails tests. What am I missing?

@vlad17
Copy link
Owner

vlad17 commented May 13, 2024

Which tests fail? Any message?

@ovr
Copy link
Contributor Author

ovr commented May 13, 2024

@timbray FYI: git clone -b fix/macos-build [email protected]:cube-js/datasketches-rs.git

Don't forget to do brew install coreutils

@timbray
Copy link

timbray commented May 13, 2024

D'oh, I'd forgotten you can clone a branch. I was fighting with "checkout" and so on. Anyhow, no luck:

Unfortunately, looks about the same. See https://gist.github.com/timbray/3b4c3117a9ee82b797bee8ee8a39f36c

@ovr
Copy link
Contributor Author

ovr commented May 13, 2024

/bin/bash: guniq: command not found

Don't forget to do brew install coreutils

@timbray

@timbray
Copy link

timbray commented May 13, 2024

Oh, that did it, pardon me missing that. All tests now passed.

@vlad17
Copy link
Owner

vlad17 commented May 13, 2024

Excellent. Thank you both for following up. @ovr , mind updating the readme to mention brew install coreutils for Mac for unit tests? Then should be good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants