-
Notifications
You must be signed in to change notification settings - Fork 33
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
feature/bazel #357
feature/bazel #357
Conversation
Feature/bazel overrides
feature/bazel-explain
buckaroo/DefaultSourceExplorer.fs
Outdated
@@ -6,7 +6,7 @@ open Buckaroo.Console | |||
open Buckaroo.RichOutput | |||
open Buckaroo.Logger | |||
|
|||
type DefaultSourceExplorer (console : ConsoleManager, downloadManager : DownloadManager, gitManager : GitManager) = | |||
type DefaultSourceExplorer (console : ConsoleManager, downloadManager : DownloadManager, gitManager : GitManager, buildSystem : BuildSystem) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does the source explorer need to know about the buildsystem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Bazel mode we only require a WORKSPACE
file to be present. See #358
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit arbitrary and we had users in the past asking for this feature when using Buckaroo without a buildsystem. I'd prefer if we could enable this behaviour independent of the buildsystem.Maybe this should be part of the PackageLocation type?
Given how we fetch the manifest this would be quite easy to implement and would enable to handle npm, java etc in a unified fashion here:
https://github.com/LoopPerfect/buckaroo/blob/master/buckaroo/DefaultSourceExplorer.fs#L293
This would enable us to easily extend the way how we can handle deps with non-buckaroo manifests:
buckaroo add nomanifest+gh+google/tensorflow
buckaroo add npm+gh+facebook/react
buckaroo add pom+gh+google/guava
buckaroo add pom+mvn+google/guava
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this require more effort on the part of the user to annotate all of the Bazel (but not Buckaroo) packages they want to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but that is also a matter of choosing defaults.
If we handle Build Systems, packaging formats and constraintsas seperate concerns we can then
handle multi language / multi buildsystem projects more elegantly.
We can follow up with UX features that modify the default behaviour of buckaroo add
via eg. Environment variables, .buckaroorc, existence of bazel files (buckaroo init --bazel
?)
Bazel (but not Buckaroo) packages
In future, every library that can be installed by Buckaroo should be considered a Buckaroo package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying the behaviour of this PR is incorrect (from a user's perspective) or that the implementation is not (elegantly) extensible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both:
the behaviour of how to retrieve (or what should happen if you can't) a manifest of a specific PackageLocation is a property of that specific package and not a (global) property of the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Can you write up a quick Hack MD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BuildSystem param now removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything else looks good great
Rudimentary Bazel support.