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

Support for automatic activation of profiles via new :active? expression #1228

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dobladez
Copy link

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

@hypirion
Copy link
Collaborator

Not using the metadata constraints usage: How would you represent the profile :foo [:bar :baz :quux] as an active profile? That's simply not possible, as vectors can't map :active? to a value. This will be possible if it's a metadata argument.

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.

@dobladez
Copy link
Author

Right. The way I implemented it, it doesn't honor the :active? rule on composite profiles. That said, it's more of a bug on the impl than an impossibility. This should have worked:
:foo [:bar :baz :quux { :active? true, profile-specifics-here "blah" } ]

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 :profile-activations holding such rules. May be:

:profile-activations {:foo (a-predicate )
                      :bar (a-predicate )}

Thanks

@dobladez
Copy link
Author

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:

 :foo [:bar :baz :quux]

and my activation rules say that :baz must be inactive, then I would like lein with-profile foo pprint to activate :foo, :bar and :quux (but not :baz).

Note: From what I tried, looks like the +/- prefixes on with-profile don't support this either: if you do +foo,-baz, it activates all of them, including :baz.

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 lein with-profile foo pprint will always activate :foo itself and only :foo-linux or :foo-mac depending on whether their :active? expressions returned true or false.

am I clear? Can you think of a cleaner way to achieve the same?

thanks

@hypirion
Copy link
Collaborator

@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 foo, you will only include relevant profiles, and if you're using, say foo,+foo-linux, you will get foo-linux regardless of what os you're using. (Otherwise I'm afraid people will get a nasty surprise when trying to apply profiles.)

Thing is, keywords aren't able to hold metadata. In that case, either replacing ^{:active? (test-if-linux)} :foo-linux with ^{:active? (test-if-linux)} [:foo-linux] may be an option, albeit a bit ugly. I'm not sure yet.

@technomancy
Copy link
Owner

Sorry for the delay here in chiming in. Overall I think it looks pretty good.

Whether a profile is active or not is not a property of the profile, but the context it is in.

What if we used the :activate? key instead of :active?? That implies we're sending a directive to Leiningen to activate it on our behalf rather than activating it straight away, which makes more sense given that there are ways to disable its activation.

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 with-profiles? It looks like the auto-activation stuff only applies to the initial read of the project? So not only will lein with-profiles -auto ... deactivate it, but lein with-profile production ... will as well, right? Is that intended?

@dobladez
Copy link
Author

Yes, I like :activate? better. Metadata or plain key as in the current patch?

It interacts with with-profile the way you describe it. However, that's not what I want now that I think about it. I think with-profile otherprofile should not deactivate the implicit ones.

@dobladez
Copy link
Author

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 :activate? is true.
I haven't looked at how hard this would be to implement... but I like this better than my original proposal. It's more general.

@hypirion
Copy link
Collaborator

@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 with-profile logic, I'm still a bit confused on how to do it properly, but I think the following may be reasonable logic and should cover most use cases (?):

Only top-level profiles (those residing within :profile/profiles.d/profiles.clj) with an :activate? value which is truthy will be automatically activated. Top-level autoactivated profiles can only be deactivated by explicitly saying with-profile -foo. Activating an autoactivated profile will do nothing (unless it has been deactivated in an outer scope).

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 :activate? value is truthy or if the :activate? key is nonexistant. Profiles where the :activate? value is falsey will not be activated.

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.

@technomancy
Copy link
Owner

I'm leaning slightly towards metadata over keeping :activate? in the map, but I don't feel too strongly about it.

Does the proposed functionality in the :specialprofile example above replace the implementation in the patch? I'm not quite as keen on it since it means a single profile is composed of subcomponents that don't exist as first-class entities on their own. There's definitely something to be said for ensuring that each map has a name and can be inspected via things like the show-profiles task.

@hypirion
Copy link
Collaborator

@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

  1. All profiles should have names
  2. It should be possible to autoactivate profiles
  3. When activating a composite profile, it would be valuable to only activate parts of the profile
  4. Sensible semantics on how to turn an auto(de)activated on and off with with-profile

Have I forgotten something?

The cleanest solution I can see here is tagging :active? as metadata onto the profile. This would also allow composite profiles, like e.g. this:

: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.

@technomancy
Copy link
Owner

Sensible semantics on how to turn an auto(de)activated on and off with with-profile

So @dobladez says an explicit design goal is "I think with-profile otherprofile should not deactivate the implicit ones."

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 :activate? predicate return false. The former is much simpler to implement, but as long as it's well-documented I don't have a problem with the latter as it sounds like it fits the original use case better. Trying to create some kind of in between mechanism would make things too difficult to follow IMO.

When activating a composite profile, it would be valuable to only activate parts of the profile

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.

@technomancy
Copy link
Owner

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?

@llasram
Copy link
Collaborator

llasram commented Aug 31, 2013

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?

@hypirion
Copy link
Collaborator

hypirion commented Sep 1, 2013

@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.

@Jackywmf
Copy link

Jackywmf commented Jul 5, 2014

Support

technomancy added a commit that referenced this pull request Aug 9, 2014
@technomancy
Copy link
Owner

What do you think about the solution documented in 9750d8e?

@hypirion
Copy link
Collaborator

hypirion commented Aug 9, 2014

@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 lein with-profile [+-]os ...?

@technomancy
Copy link
Owner

@hypirion that might be nice. If the primary use case is for OS, then changing the definition of :default to something like [:base :system (symbol "os" (leiningen.core.utils/get-os)) :user :provided :dev] might be good, (provided we suppress "profile not found" warnings for things starting with :os) but if it's common to use it for other things then maybe it's best to keep it general.

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.

5 participants