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

Sort out recursive/non-recursive aliases #268

Merged
merged 13 commits into from
Oct 19, 2017
Merged

Conversation

ghost
Copy link

@ghost ghost commented Sep 29, 2017

Fixes #253.

ThisPR removes the special recursive aliases runtest, install and doc. Now all aliases are non-recursive. However, the interpretation of aliases on the command line is changed so that when the user types:

$ jbuilder build @src/x

this is interpreted as alias x in all descendant of src where it is defined. This way the interpretation of @runtest, @install and @doc is unchanged and users can define their own global aliases such as examples, runtest-js, ...

Inside jbuild files, one can use (deps (... (alias_rec XXX) ...)) to get the same behavior.

In any case, when the alias is defined in none of the descendant of the specified directory, jbuilder reports an error. Except for runtest, install and doc as this could break existing invocations of jbuilder.

Interpret then while loading the file tree.
Calling 'jbuilder build @path/x' always request the alias `x` in
`path` and all its descendant.

To implement that, change the build system interface to take an
arbitrary request as argument.
(alias_rec XXX) means the same as @xxx on the command line.
src/alias.ml Outdated
~else_:(Build.arr (fun x -> x)))
>>^ fun is_empty ->
if is_empty && not (is_standard name) then
Loc.fail loc "This recursive alias is empty.\n\
Copy link
Member

Choose a reason for hiding this comment

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

"recursive" should be probably be removed from this error message if recursive aliases no longer exist.

@rgrinberg
Copy link
Member

@bobot @dra27 I've resolved the trivial merge conflicts and reviewed this PR. Jeremie also indicated that it was ready. Will give you two a couple more days to indicate interest in reviewing this, otherwise I will merge.

let path = File_tree.Dir.path dir in
let t = of_path (Path.relative path name) in
acc
>>>
Copy link
Member

Choose a reason for hiding this comment

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

Curious if the sequencing here is actually necessary. Seems like this could be done in parallel?

@rgrinberg
Copy link
Member

rgrinberg commented Oct 16, 2017

The CI failure is concerning. Jbuilder is running the same commands but in a different (also correct) order in the jsoo test. So there's no bug, but the tests are a bit overly fragile on this.

Whoops, I just forgot to update the test output locally.

The order of some commands changed. However, the same commands are still being
executed.
@rgrinberg
Copy link
Member

I've added some pretty printers to this PR that helped me understand how the alias stuff works. It's very useful to be able to inspect the alias store. Others might find it useful as well.

@rgrinberg rgrinberg merged commit a63276f into master Oct 19, 2017
@rgrinberg rgrinberg deleted the new-alias-semantic branch October 19, 2017 07:33
per-directory. However, on the command line, asking for an alias to
be built in a given directory will trigger the construction of the
alias in all children directories recursively. Jbuilder defines the
following standard aliases:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you also document the non recursive use on the command line. If you write jbuilder build src/x it means just build the alias x in directory src. The API seems to allow it no?

Copy link
Member

Choose a reason for hiding this comment

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

Did you mean @src/x? I'm not aware that it's possible to ask for an alias to be built without it. I just gave it a try as well and it doesn't seem work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, so there is no way to call for an alias from the command line in a non-recursive way. Perhaps it would be good to add that in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I personally don't have much of a need for non recursive invocations. But sure, if it's easy, let's add it.

@bobot
Copy link
Collaborator

bobot commented Oct 19, 2017

Sorry It seems I forgot to click a second time to submit the review, that I have done some days ago...

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