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

[draft] OSS builds #88

Closed
wants to merge 6 commits into from
Closed

Conversation

gabrielrussoc
Copy link

  • Fixed branches from master to main.
  • Removed FB only internal code
  • Removed fb feature
  • Some hacks to point to github crates instead of in-repo crates

@gabrielrussoc gabrielrussoc mentioned this pull request Nov 7, 2021
Copy link
Member

@fanzeyi fanzeyi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I left a few comments.

Curious: are you working on this on behalf of Databricks or in personal capacity?

Comment on lines -790 to -795
#ifdef EDEN_HAVE_SERVICEROUTER
ConfigSetting<bool> enableServiceRouter{
"facebook:enable-service-router",
fb_has_servicerouter(),
this};
#else
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we define this for open source build? If it is defined we might want to fix that instead of removing this line.

@@ -17,7 +17,7 @@
#include <folly/portability/SysTypes.h>
#include <folly/portability/Unistd.h>

#include "common/rust/shed/hostcaps/hostcaps.h"
//#include "common/rust/shed/hostcaps/hostcaps.h"
Copy link
Member

Choose a reason for hiding this comment

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

Instead of removing it, maybe

Suggested change
//#include "common/rust/shed/hostcaps/hostcaps.h"
#ifdef EDEN_HAVE_SERVICEROUTER
#include "common/rust/shed/hostcaps/hostcaps.h"
#endif

@@ -3,7 +3,7 @@
# This software may be used and distributed according to the terms of the
# GNU General Public License version 2.

rust_static_library(rust_backingstore CRATE backingstore FEATURES fb)
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this optional? I think we have a CMake variable to check if it is Facebook internal build or not.

@gabrielrussoc
Copy link
Author

gabrielrussoc commented Nov 17, 2021

@fanzeyi We (Databricks) are looking into ways to improve our monorepo performance and Eden looks promising.

This PR was a step I took to validate if it actually works outside of FB and how supported it is. Do you know if anyone has ever deployed Eden outside of FB?

@fanzeyi
Copy link
Member

fanzeyi commented Nov 17, 2021

@gabrielrussoc Ah, that's cool! I interned there a few years ago. 😀

I don't think anyone has deployed EdenFS outside Facebook. The git support we have is very basic (if that's the route you are considering).

We do deploy the CMake build internally on certain platforms because of build system limitations, so it mostly work other than these corners where FB environment is expected.

@gabrielrussoc
Copy link
Author

@fanzeyi I interned at FB a few year ago too 😅

The git support we have is very basic (if that's the route you are considering).

It's not very clear to me what kind of Git support there is. Let me open up an issue for better visibility and ask a few questions there

@ahornby
Copy link
Contributor

ahornby commented Feb 15, 2022

OSS builds are working again

@ahornby ahornby closed this Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants