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

Add support for using the CLJ_CONFIG env var. #34

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

Conversation

sundbp
Copy link

@sundbp sundbp commented Apr 2, 2019

This is very helpful for e.g. certain monorepo workflows.

This is very helpful for e.g. certain monorepo workflows.
@SevereOverfl0w
Copy link
Collaborator

I'm not sure about merging this.

I know some people are (ab)using CLJ_CONFIG for monorepo setups, but I haven't actually found it necessary at all with Edge. If someone actually were using CLJ_CONFIG (eg for development purposes) then this would cause dev dependencies created on their machine to differ from other people's.

I expect @AlexMiller will create an alternative solution to solve the monorepo problems more generally in the future.

@sundbp
Copy link
Author

sundbp commented Apr 2, 2019

I think there are a few things here:

  • not using CLJ_CONFIG makes it inconsistent with the rest of the tools.deps ecosystem, and hence one gets unexpected behaviour
  • I do (ab)use CLJ_CONFIG to unify versions across a monorepo. I wrap my tasks (e.g. repl, uberjar'ing etc) in a way to ensure consistent and reproducible behaviour (incl CLJ_CONFIG). That seems a reasonable trade-off - I get version consistency, I give up other use of CLJ_CONFIG until there is a better monorepo story around.
  • Edge has version duplication pretty much everywhere, one of the main reasons I don't find it appealing to work with, while I do share a lot of other ideas it uses. I basically use something taking ideas from edge and from seancorfield's CLJ_CONFIG "trick/hack/workaround" for version handling.

As part of this discussion I feel like the first point is the most objective one - inconsistency is never a good thing. I personally then feel the other 2 points relate to the subjective view of allowing users to shoot themselves in the foot and make their own risk/return decisions.

Even if we had some other magical feature for better monorepo support I'd argue it makes sense for consistent use of CLJ_CONFIG to avoid surprises. I'm sure there will be better support for monorepos, but I don't think that makes this PR void.

@SevereOverfl0w
Copy link
Collaborator

I believe that packs behaviour is consistent with -Srepro, which is the intention.

@sundbp
Copy link
Author

sundbp commented Apr 2, 2019

It is - but without the user having passed -Srepro asking for it. That choice was taken away from from the user. My patch should be updated to suppress CLJ_CONFIG when -Srepro is indeed passed. Would that be a good trade-off? Or if you really hate using the same semantics leading to it being on by default, making it off by default but adding a pack flag to enable it?

@SevereOverfl0w
Copy link
Collaborator

The thought process is that release artefacts are a sensitive output. Reproducibility is an important property. So factoring in the user deps.edn file is a no no.

I don't have a good idea yet of what I think should happen here. Maybe supporting -Sdeps would resolve this?

Alternatively, maybe this is an opportunity to expose more of an API interface from pack? Then users can layer on as many deps.edn files as they like.

@sundbp
Copy link
Author

sundbp commented Apr 2, 2019

I agree reproducibility is an important feature. I enforce mine through a pack script (e.g. I use no pack alias or anything like that at all, it's all in the pack script), which will indeed make my builds reproducible. In my setup the CLJ_CONFIG is not a user file, it's a monorepo file under control of the monorepo. -Sdeps is in principle an escape hatch, but it wont be smooth as it'd require e.g. reading in a deps.edn file and pass via -Sdeps (with potential quoting issues etc). Some similar in spirit flag to indeed add in "any" deps.edn file would solve my situation.

In general, for clj itself as well, if I had a flag to merge in another arbitrary deps.edn file the monorepo case would "work". I use CLJ_CONFIG as my "extra" deps.edn, and I'd gladly use a different way to get the extra deps.edn into the mix without "borrowing" CLJ_CONFIG to be a monorepo setting.

@SevereOverfl0w
Copy link
Collaborator

Fwiw, quoting isn't too difficult here because bash takes care of that.

Generally, clj -Sdeps "$(< ../extra-deps.edn)" works with clj itself without issue, nothing scarier than standard bash escapes.

@sundbp
Copy link
Author

sundbp commented Apr 2, 2019

To make matters worse I need both linux and windows support :/ I still haven't worked out the cmd/powershell stuff properly. But that's encouraging.

A flag to add a deps.edn file feels like it could be a decent way forward. I'll mention it to Alex and see if it's something considered or worthy of consideration for clj itself.

@SevereOverfl0w
Copy link
Collaborator

What was the response from Alex regarding a flag for providing a file to clj?

@SevereOverfl0w
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants