Restrict profiles.d files so that the filename matches the profile name? #1068

Closed
djpowell opened this Issue Mar 16, 2013 · 14 comments

Projects

None yet

5 participants

@djpowell

There are instructions everywhere describing how to install plugins by editing the :user profile ~/.lein/profiles.clj

But in 2.0.0, profiles.d means that the :user profile could be in profiles.clj, or in any file under profiles.d. But profiles can only exist in one location without an error being thrown.

This seems like it has the potential to confuse people.

Additionally, I'd like to be able to set :java-cmd in :user in my Windows installer. I'm looking at using something like https://github.com/cgrand/sjacket to parse the profile and insert, or change the variable without messing up comments, layout, and metadata.
But because of profiles.d I don't know where :user might be located.

Proposal:

1) Restrict user-level profiles to being located in either:

a) ~/.lein/profiles.clj

or

b) ~/.lein/profiles.d/.clj

This limits each profiles.d file to containing a single profile.

2) Blacklist default profiles (user, dev...) from appearing in profiles.d

@michaelklishin
Collaborator

But what's the ultimate goal? why do you need to know where exactly does :user profile come from?

@djpowell

There are probably two separate issues here.

1.
In the general case (ignoring my desires to find where a profile came from) allowing builtin profiles to occur in profiles.d is error prone and not very useful.

The purpose of profiles.d seems to be to make it easier to install profiles by dropping them into a directory. This is great for custom profiles.

However the merge strategy is to error if a profile exists in more than one source. So it would be a bad idea to put builtin profiles like :user in profiles.d, because then anyone following standard practice to install plugins by editing profiles.clj will be met by errors, and they'll need to know to how to fix the problem by trawling through profiles.d looking for the duplicate definition, and moving the declarations into one file or the other. This is the sort of thing that profiles.d was trying to prevent, but for builtin profiles it could end up making things worse.

So for this reason, I think we should disallow built in profiles from profiles.d

2.
I specifically would like to be able to find the user profile containing :user so that my Windows leiningen installer can modify the file to update the :java-cmd definition to point to the user's preferred JDK. This isn't impossible as-is, but it would be easier if I didn't have to parse every file under profiles.d to look for the definition, especially when defining built-ins in profiles.d seems to be a bad idea to begin with.

Given the error-on-merge strategy, putting each profile in its own file seems a good idea, as it guarantees that no merge errors can occur between profiles.d files, and makes managing them easier.

@technomancy
Owner

I'm not too keen on disallowing overriding of built-in profiles. We're making some decisions about what profiles should ship out of the box, but I'm not 100% confident they're right for everyone, so people should have the power to override them. However, we could switch to namespace-qualified keywords for built-in profile names if you're worried about conflict.

There's a special case of :dev in user-level profiles which is always a mistake; I have no problems with issuing a warning when this is detected. The dev profile is designed to only be included in project.clj.

I am fine with the idea of only looking for profiles in the file named after said profiles inside ~/.lein/profiles.d/; that makes sense. Maybe @dakrone could chime in since this was his idea originally.

@technomancy
Owner

If files in profiles.d may only contain one profile, then maybe it should be as a top-level map rather than the same format as profiles.clj which maps profile names to profile maps.

@djpowell

There are two main use-cases of profiles.d:

1) It allows a user to split their profiles into different files if they think that is tidier / prettier.

2) It allows a user, or an agent on the behalf of a user, to deploy custom profiles by dropping them into profiles.d

The original enhancement issue discusses case 2).

I think it is useful to think about the who more than the what.

If we were only dealing with case 1), then there wouldn't be a problem. The user could make sure that they didn't have any clashing profiles between profiles.clj and profiles.d.

But for case 2) to work, the user or agent needs to be confident that the profiles that they are dropping into profiles.d aren't going to clash with profiles in profiles.clj, else things will break.

Given the near certaintly of :user existing in profiles.clj, I think that we should ban :user from profiles.d.

:dev is already banned from the profiles.clj (and I assume profiles.d?).

I'm not as concerned about the other built-ins.

@djpowell

tldr; #871 is about drop-in custom profiles, which have benefits and no real costs;
but for built-ins the only benefit is a tidier directory structure, but the cost is making things potentially more error prone and confusing

@technomancy
Owner

Oh; I see, you're using "built-in" differently than I am. I meant the profiles that ship with Leiningen itself, so :base, :test, and :repl but you're talking about those which are activated by default?

IIRC the original motivation of profiles.d was actually to allow you to separate sensitive settings out from the other profiles so that the non-sensitive bits can be checked into version control; it wasn't designed for programmatic profile creation.

@dakrone
Collaborator

Yes, if the profiles.d directory were restricted to the name of the profile, we wouldn't be able to have separate profile names for our work settings for example.

@djpowell

@dakrone why not? Couldn't you just deploy them in two separate files?

@technomancy I'm not sure, I only really use the :user profile. My main concern is that every plugin documentation describes how to install plugins by editing :user in profiles.clj, and I don't want to the instructions to say: "edit profiles.clj, unless one of the files in profiles.d has the :user profile defined, in which case you must edit it there else you'll get an error". Cause that is just creates a lot of pain, and that flexibility doesn't really benefit anyone.

@technomancy
Owner

@djpowell Good point about all the existing documentation. With that in mind I'd be fine restricting the :user profile to profiles.clj.

I don't understand the problem @dakrone mentioned unless he specifically wants to avoid checking in multiple files or something.

@hyPiRion
Collaborator

I'd be fine with user being put in profiles.clj only. Seems reasonable.

But if an IDE or similar ships with multiple profiles and we want to limit it to one profile, we should issue a warning about this being deprecated before breaking people's profiles.d directories. We can break it for 3.0.0.

@djpowell
@hyPiRion
Collaborator

Ah, you're right, disregard my comment.

@technomancy
Owner

From IRC:

<technomancy> djpowell: ok, so my thoughts are basically that I like the
              idea of no map wrapper and that I'm not 100% sure about the
              limitation on the user profile, but the great thing about
              restrictions is that you can lift them in the future without
              breaking compatibility
<technomancy> so I'm ok with introducing profiles.d with this restriction
              given that we can revisit it to possibly make it less picky
              in the future; I don't think we necessarily have to make a
              Final Choice for 2.1.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment