-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Support for automatic activation of profiles via new :active?
expression
#1228
base: master
Are you sure you want to change the base?
Conversation
Not using the metadata constraints usage: How would you represent the profile I also believe it's semantically unsound. Whether a profile is active or not is not a property of the profile, but the context it is in. I can see the use case of having this in though, I just think it makes more sense to have it as metadata. That's my 2 cents. |
Right. The way I implemented it, it doesn't honor the anyway, I don't like it either :-) We could go with metadata, or, to throw another option, we could have a separate project key. Something like
Thanks |
Here's a more demanding requirement: This implementation doesn't handle composites in a situation I just run into: I'd like these rules to also be able force the exclusion of a profile, such that if I have the following:
and my activation rules say that Note: From what I tried, looks like the +/- prefixes on The real use case? I have a profile that defines new dependencies. One of those dependencies (plus a JVM property) is OS-dependent. So, I here's my current idea of how this could be solved: :profiles {
:foo [ :foo-linux :foo-mac { #_ common-deps-here } ]
:foo-linux ^{:active? (= (System/getProperty "os.name") "Linux") } { #_ linux-specifics-here }
:foo-mac ^{:active? (= (System/getProperty "os.name") "Mac OS X") } { #_ mac-specifics-here } } and then hoping that am I clear? Can you think of a cleaner way to achieve the same? thanks |
@dobladez, I think @technomancy should get a word here, he may have some ideas on a good approach. As for the activation-logic: Hmm, this is hard. I was kind of hoping that there would be some way to do, say :profiles { :foo [^{:active? (test-if-linux)} :foo-linux
^{:active? (test-if-mac)} :foo-mac
:other-deps-here]
:foo-linux ^{:active? (test-if-linux)} {:linux-specific :stuff}
:foo-mac {:mac-specific :stuff}} So that when using Thing is, keywords aren't able to hold metadata. In that case, either replacing |
Sorry for the delay here in chiming in. Overall I think it looks pretty good.
What if we used the Personally I'm OK with punting on composite profiles; I don't see a tidy way to handle that. Can you clarify a bit about how this interacts with |
Yes, I like It interacts with |
Here's another idea: try to resolve this at a lower level: "within" a profile. Example: (I know lein uses vectors for composites today... I'm just throwing something to start the discussion): :specialprofile [{:dependencies [...]
:these-are-common-keys "blah" }
{:activate? (predicateA ...)
:dependencies [...] }
{:activate? (predicateB ...)
:dependencies [...] }] So, when the profile is read, lein merges the maps for which |
@technomancy yeah, it's a better description. I think me discussing whether to contain it in the map or as metadata was just me being in a grumpy/nitpicking mode, the functionality itself shouldn't change regardless of whether the data is meta or not. As for the Only top-level profiles (those residing within Profiles residing directly within other profiles (such as @dobladez' example in his last post) will only activate when its parent profile is activated and its Referring to a top-level profile within a "vector"-profile will always turn it on, regardless of its activation status. (It makes no sense to refer to a top-level profile otherwise, because keeping them in the top-level would automatically turn them on). Edit: I have probably forgotten something, so if anything can comment on what I've forgotten, shout out. |
I'm leaning slightly towards metadata over keeping Does the proposed functionality in the |
@technomancy I agree that having named profiles is valuable. Not only for users, but it would help us not obliterate the remaining sensible code we have in profile merging. So, what we would like is essentially
Have I forgotten something? The cleanest solution I can see here is tagging :profiles { :foo [^{:active? (test-if-linux)} [:foo-linux :more-linux-profiles :here]
^{:active? (test-if-mac)} [:foo-mac]
:other-profiles-here]
:foo-linux ^{:active? (test-if-linux)} {:linux-specific :stuff}
:foo-mac {:mac-specific :stuff}} Not sure if the composite profile autoactivation is a good solution, but I don't think we have to decide that immediately. We can always defer that for a later version. |
So @dobladez says an explicit design goal is "I think I think for simplicity's sake we should either make it so implicit profiles get deactivated the same as the built-in default ones or make it so implicit profiles can only be deactivated by causing their
I agree that we can punt on this; let's keep the current proposal focused. If we get the deactivation sorted out we should be able to get this into 2.3.0 and released soon. |
Hey, we're thinking of cutting a release for 2.3.0 soon. Should we target this feature for the release, or push it off to the next one? |
Bump. I'm hitting a need to have differing profiles per JVM version, and it would be really nice if they could auto-apply according to the detected JVM version. It looks like this feature would cover it nicely. Has there been any more discussion out-of-PR or how the merge-able version of this feature should work? |
@llasram no, it has been some time since this discussion took place. Phil mentioned that he's "not too keen on having them stay active when you choose a new set of profiles via with-profiles" on IRC to you. We can still discuss how the functionality of automatic activation will work here though, and figure out the design of this. |
Support |
What do you think about the solution documented in 9750d8e? |
@technomancy Oh, that's a neat way of solving the problem. Would it perhaps be smarter to tell people to do :profiles {:dev [:dev/all :os]
:dev/all {...}
:os [~(leiningen.core.utils/get-os)]
:linux {...}
:windows {...}
:macosx {...}} so that you can actually deactivate/reactivate the OS-specific functionality through |
@hypirion that might be nice. If the primary use case is for OS, then changing the definition of |
My immediate use-case for this feature is to be able to automatically activate different profiles depending on which OS lein is running.
I opted for a more general solution so that the user can use any activation predicate.
As for the syntax, I just picked one approach of may options. Instead of using :active? we could have used it as metadata ^{:active expr }, which is probably more "correct", but I thought it added quite a bit of noise to the project definition (specially for non-Clojure experts)
I also added a pointer on PROFILES.md to the
with-profile
docs, because that page didn't say anything about prefixing profile names with-
/+
.Thanks,
Fernando