search

privacylab / talek

a Private Publish Subscribe System
BSD 2-Clause "Simplified" License
47 stars 5 forks source link

cuda/cl tags should be opt-in #64

Open willscott opened 7 years ago

willscott commented 7 years ago

having dealt with this for a bit now, i've decided i care more, and that the optional libraries need to be opt-in parts of the build.

editors / standard ecosystem tools expect a 'go build' without tags to succeed, and tags are not always available. (e.g. atom doesn't have a way to set them https://www.bountysource.com/issues/42210626-does-go-plus-support-build-tags)

If i can't run go build successfully in the project because it fails on cuda headers that i can't install / test because my computer doesn't support them, i'm just not going to run the tests.

ryscheng commented 7 years ago

To summarize my reason for going with opt-out: The high level goal is to be running as many of the tests as possible. It seems way too easy to make a bunch of opt-in build tags that we just end up forgetting to build or test. There's an example of how we opt-out of CUDA/CL tests in the Travis configuration.

I'd be happy to support this issue if we get a CI environment that can run all the tests, so that this concern is alleviated. I've opened this issue to track: https://github.com/privacylab/talek/issues/67

In the meantime, you can easily setup your own test scripts in atom: https://stackoverflow.com/questions/39146511/how-to-setup-a-custom-keybinding-to-run-a-script-or-execute-a-command-in-atom-ed

willscott commented 7 years ago

test scripts don't solve this: the 'go plus' environment is what hooks in to the editor with inline coverage, refactoring, and error flagging. none of those happen if the build fails because cuda headers are missing.

ryscheng commented 7 years ago

Okay, I see the problem. Seems a little silly for go-plus to not support custom build tags. https://github.com/joefitzgerald/go-plus/issues/573 https://github.com/joefitzgerald/go-plus/issues/603

In my opinion, whether it's opt-out or opt-in should be independent of whether this Atom package properly let's you configure your go environment.

willscott commented 7 years ago

i'm sympathetic to wanting to make sure that code stays alive and gets tested. at the same time, it feels like it's hopefully already protected by a pretty static API that isolates it, and all of the development we're going from here on out doesn't change what that small kernel of GPU work is.

having that be an imposition on work on the client, frontend, coordinator, or distributing replicas across machines is gonna get old.

willscott commented 7 years ago

noting for myself that this caused https://github.com/privacylab/talek/commit/a6ffbc62b07f26c497b76a2e79200dbdc0929101

Atom didn't catch it because it was stuck on not finding cuda.h, and makefile failed as well because of tag errors.

If i run into this again i break down and try to fix the environment and isolate the optional dependencies.