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

[POC] Elm json "source-directories" support #27

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

turboMaCk
Copy link
Contributor

@turboMaCk turboMaCk commented May 19, 2019

At this moment this is only proof of concept that needs to be discussed and polished.

Related to #6 and #19

This is an attempt to implement source-directories (elm.json) compatibility for both code generation and nix derivations.

Background

Elm make performs lookups for modules based on optional source-directories attr in elm.json file. If this attribute is not present ./src is a default destination where it looks for .elm files.

  • it's possible to import foo/A.elm within bar/B.elm as import Aand import B if both foo and bar are defined in source-directories
  • it's possible to do elm make foo/A.elm bar/B.elm as well (as an entry).
  • it's possible to use any relative path like ../../baz

Implementation

Because of this, we need to use srcs instead of src. srcs attribute is currently generated in Haskell from elm.json. Paths like ../foo are also valid and needs to be supported. In the case of . (current directory) nix seems to be using the name of the current directory. This means that ["../foo", "."] within app results in 2 directories foo app. Because of changes in directories structure, new elm.json must be produced during the build - done via nix.

Details will be in source comments.

.gitignore Outdated Show resolved Hide resolved
shell.nix Outdated Show resolved Hide resolved
Copy link
Contributor Author

@turboMaCk turboMaCk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented not so obvious parts of the diff.

data/default.nix Outdated Show resolved Hide resolved
data/default.nix Outdated Show resolved Hide resolved
@@ -6,27 +6,47 @@ with (import nixpkgs config);

let
mkDerivation =
{ srcs ? ./elm-srcs.nix
, src
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced by srcs

, targets ? []
, versionsDat ? ./versions.dat
}:
stdenv.mkDerivation {
inherit name src;
inherit name srcs;
sourceRoot = ".";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be hardcoded - we place generated elm.json with correct paths and the prefix elm make with right dirs.

data/default.nix Outdated Show resolved Hide resolved
data/default.nix Outdated Show resolved Hide resolved
data/default.nix Outdated Show resolved Hide resolved
-- TODO: there is likely to be even better abstraction
tryLookup :: HashMap Text Value -> Text -> Either Elm2NixError Value
tryLookup hm key = maybeToRight (KeyNotFound key) (HM.lookup key hm)
where
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reused between parseElmJsonDeps and parseElmJsonSrcs

@turboMaCk turboMaCk force-pushed the better-support-for-source-dirs branch from ce64e0f to f7b8cc8 Compare May 23, 2019 16:10
srcdir = "${srcdir}";
targets = ["./Main"];
Copy link
Contributor Author

@turboMaCk turboMaCk May 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BREAKING change: Main.Entry => ./Main/Entry. Supports custom extensions like Main.js.elm or Main.html.elm and entries from different sources (relative path from current dir). UPDATE: elm compiler will no longer support entries like this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we revert this then?

Copy link
Contributor Author

@turboMaCk turboMaCk Sep 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunetely this is still required.

Imagine you have source-directories like this defined in elm.json:

"source-directories": [
  "./src",
  "./src2",
  "../lib"
]

Now when you set targets = [ "Main" ] it's not clear in which directory this main should be located. It can be any one of those.

Another issue is that with current dir. Imagine having this elm.json:

"source-directories": [
   ".",
   "../lib"
]

since I've used srcs attribute in derivation nix will create structure as:

  • $pwd # contains files form ., is named after current durectory
  • lib # contains files from ../lib

this is why Main is prefixed with ./. We need to replace this ./ prefix with the name of directory to which builder placed actual source from current dir.

I wonder if there is any builtin helper that would help to resolve paths in a way nix does it when srcs attribute on mkDerivation is used. I belive this is done in a this shellscript

pure (Vector.head dirs)

lastDir :: FilePath -> FilePath
lastDir = foldl (\path c -> if c == '/' then "" else path <> [c]) ""
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not the most efficient way to implement this but quite concise

@turboMaCk
Copy link
Contributor Author

@domenkozar This is ready for a review.

principles followed:

  • as simple and concise as I managed to make it
  • pkgs.mkDerivation of default.nix doesn't contain any hs generated code - all project specific tab is argument defined in in block
  • as much breaking changes as possible

Some of the functions are not exactly most optimal or robust (path handling mostly).

@turboMaCk turboMaCk changed the title POC "source-directories" support Elm json "source-directories" support May 25, 2019
"type": "application",
"source-directories": [
".",
"../b-src/src"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests #6

{
"type": "application",
"source-directories": [
".",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests #19

stringifySrcs xs =
"[\n"
<> foldr (\i acc -> " " <> i <> "\n" <> acc) "" xs
<> " ]"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: use QuasiQuotes

@domenkozar
Copy link
Member

Sorry for taking this long, I've just moved over the continent and don't have immediate time to review this work. Hope you're ok waiting a few more days or so.

@turboMaCk
Copy link
Contributor Author

no worries. I'm busy as well. Take your time, there is no need to rush anything. Given the nature of these changes, I expect this to take maybe even a few weeks still

@turboMaCk
Copy link
Contributor Author

turboMaCk commented Sep 29, 2019

This is how it works on a project with rather complicated source-directories setting in elm.json:

elm2nix-srcs2

That's being said this is still not good enough for merge.

@turboMaCk turboMaCk changed the title Elm json "source-directories" support [POC] Elm json "source-directories" support Sep 29, 2019
@damien-biasotto
Copy link

What is the status of this pull request?

Also, correct me if I'm wrong, but once merged could it help us import private elm packages?

@turboMaCk
Copy link
Contributor Author

I'm not sure how you emulate private packages but if you use source-directories then yes this would solve it. But the PR is on freeze. There are few hacks that made this work and it would probably require some more discussion before being really ready. Other than that I think this PR might still work so you can try to just run elm2nix version from this.

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.

3 participants