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 an (inline ...) stanza #371

Closed
wants to merge 2 commits into from
Closed

Add an (inline ...) stanza #371

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 17, 2017

This feature adds an (inline ...) stanza whose purpose is to provide similar functionalities to (include ...), except that the inlcuded contents is written directly in the jbuild file. Jbuilder make sure that the inlined contents is kept up-to-date. This is how it works:

$ cat jbuild
(inline (echo "FOO"))
(end)
$ jbuilder build @jbuild
     patdiff (internal) (exit 1)
/home/dim/.opam/4.05.0/bin/patdiff -keep-whitespace -location-style omake -unrefined jbuild.old jbuild
------ jbuild.old
++++++ jbuild
File "jbuild.old", line 2, characters 0-1:
 |(inline (echo "FOO"))
-|(end)
+|FOO(end)
$ cat jbuild 
(inline (echo "FOO"))
FOO(end)

This is used in doc/jbuild. It should cover many cases when one has a long list of repeated rules. Compared to a classic (include ...) stanza, we have to commit the generated rules, but on the other hand it's easier to debug as we see the generated rules directly in the jbuild file.

This feature is intended for cases such as camomile, where we have a lot of rules generated, but the pattern is very specific to one use case, so it doesn't justify writing a plugin and having the actual rules under the eyes makes debugging easier.

doc/jbuild.rst Outdated
ignored. However, when building the ``jbuild`` alias, jbuilder will
run ``<action>`` and make sure that the output of the action matches
``<stanzas>``. If not, jbuilder will update the jbuild file in place
and print a diff.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some details for the doc:

  • Does jbuilder fail/succeed in that case?
  • How could one easily check that all the <stanzas> are up to date (during CI for example). ?

Copy link
Author

Choose a reason for hiding this comment

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

It fails. Jbuilder needs to be restarted to pickup the changes.

You just need to build the @jbuild alias to make sure that all the <stanzas> are up-to-date.

Copy link
Author

Choose a reason for hiding this comment

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

I updated the doc

@rgrinberg
Copy link
Member

What about using a plugin to address the intended use case with camomile? While this feature also addresses that use case, I'm not sure if it's the best option here as having all this boilerplate in your jbuild file is also kind of ugly.

@ghost
Copy link
Author

ghost commented Dec 18, 2017

Both use cases are valid I think. (inline) is best suited when the generation of the rules involves complex and/or costly computations and the result is always the same. With the plugin system as we are going to do it, at least initially, you won't be able to use arbitrary libraries in plugins, you will only be able to use other plugins.

In general let's consider the following scenario:

  • you have data files in your repository
  • you want to parse tehm and generate a list of rules that only depend on the contents of these files
  • the parsing is done by a normal library that you install with your package
  • you only need this stuff in a single project

If you go the plugin way, you have to copy the library for parsing the file inside the plugin, implement the plugin including choose a syntax for it, debug it, etc...

inline provides a simpler alternative, you just write the generator and you commit what it produces. It's easier for someone else looking at the project to understand what's going on, since they won't have to study the plugin.

In general, I expect that plugins will be better suited for more general purpose features, such as ctypes.

An alternative would be to support (include <file>) where <file> could be generated by a rule, and then you wouldn't have to commit the boilerplate. But that requires dynamic rules, which make things much more complex in jbuilder and will take more time to get in. In any case, I think it's nice to have both inline and include. Since inline makes it easier to debug problems, if we had include, we could switch between the two to debug problems.

@rgrinberg
Copy link
Member

OK, I'm convinced. I still have a couple of minor points regarding usability:

  • This is a matter of taste, but (inline ..) .. (end) syntax is really strange from a lisp standpoint. We don't want something more traditional like (inline <cmd> <generated-stanzas>)?

  • Another option is to keep the generated bits of the jbuild file in a separate file. Or perhaps keep the current mechanism but allow for a static (include ..) of a file that has (inline ..) ... (end). The reason for this is that it makes tooling a bit easier, by letting grep ignore your jbuild.generated files. I know if you have a sufficiently clever emacs setup, you can get this stuff to fold away automatically but it's worth considering making it easier for simpler setups.

Anyway, this is cool 👍

@ghost
Copy link
Author

ghost commented Dec 19, 2017

Yh, I was wondering about the syntax. Putting the generated stanzas inside the inline would make it easier to switch between include and inline as well.

The include idea make sense.This could be using a more generic feature: promotion. This is something I thought would be useful: in a user rule, you can write (promote), which means that jbuilder will generate the file, and when it doesn't match with the existing one it will:

  • print the diff and error out
  • copy over the generated file into the source tree

This is useful when you have a generator that generate static code and want to review the generated code. Moreover, coupled with an option to disable promotion, this gives you a way to cut some dependencies for the release.

So in the end you would write that:

(rule
 ((promote)
  (action (with-stdout-to jbuild.generated (run ./gen.exe)))))

(include jbuild.generated)

I guess this makes this PR obsolete.

@rgrinberg
Copy link
Member

Yes! I can already see this in a few places where an ad-hoc approach is taken to version controlling generated code.

copy over the generated file into the source tree

One last thing regarding this. Can this perhaps be generalized into some command that just accepts .correction files? I could see how such a command could be useful for linting and ppx_expect as well. Though it might require a bit more design to make it work nicely in these cases.

@ghost
Copy link
Author

ghost commented Dec 19, 2017

What do you have in mind?

@rgrinberg
Copy link
Member

Just have build generate jbuild.generated.corrected files in _build/ after failing and a simple $ jbuilder accept-corrections that will do the actual moving of these files back to the source tree.

@ghost
Copy link
Author

ghost commented Dec 19, 2017

I see, yh that does makes sense. And it moves the burden of printing the diff, choosing the diff command, etc... from the individual systems to only the build system. I like that.

To be sure we are on the same page, what the user will write is:

(rule (with-stdout-to jbuild.generated.corrected (run ./gen.exe)))

(include jbuild.generated)

And jbuilder will do something special because the target ends in .corrected.

@rgrinberg
Copy link
Member

Yes, that is what I meant.

But I did not imagine this part:

And jbuilder will do something special because the target ends in .corrected.

I still assumed people would have to mark the tests as inline manually. Though this is acceptable as well.

@bobot
Copy link
Collaborator

bobot commented Dec 19, 2017

And jbuilder will do something special because the target ends in .corrected

I agree with @rgrinberg . I prefer to have something explicit, for understandability. For example exactly what you propose, but just add a stanza for the link:

(promote (jbuild.generated jbuild.generated.corrected))

I didn't follow with which default you en up with. promote is used or not by default?

@ghost
Copy link
Author

ghost commented Dec 19, 2017

Ok to make it explicit. I think it should be used by default and we can disable it in release mode. In fact there are three possible choices:

  • promote the files
  • just check for equality
  • ignore promotion

Basically, if we write this:

(promote a as b)

then whenever the build needs a, we add the action of promoting b as a as a target. This will allow to have circular dependencies:

(executable
 ((name foo)
  (modules (foo bar))))

(rule (with-stdout-to bar.ml.gen (run ./foo.exe)))

(promote bar.ml.gen as bar.ml)

For the jbuild case, we'll write this:

(rule (with-stdout-to jbuild.inc.gen (run ./gen.exe)))

(promote jbuild.inc.gen as jbuild.inc)

(include jbuild.inc)

and jbuild.inc will always be considered as needed. Once #370 is merged, we could only do it for directories that get loaded.

Implementation-wise, we can add an action Check_corrected of Path.t * Path.t, that we can use for promote, preprocess and lint.

@bobot
Copy link
Collaborator

bobot commented Dec 19, 2017

Ok to make it explicit. I think it should be used by default and we can disable it in release mode.

What is the release mode? We already have the dev mode, I think it should be checked for equality only in dev mode.

Implementation-wise, we can add an action Check_corrected of Path.t * Path.t, that we can use for promote, preprocess and lint.

+1

@jberdine
Copy link
Contributor

What is the release mode? We already have the dev mode, I think it should be checked for equality only in dev mode.

May I just raise the point that the current hard-coded formulation of --dev makes it unsuitable in various contexts. So we end up doing "development" builds without --dev, and it would be unfortunate if this new functionality was also unusable as a result.

@ghost
Copy link
Author

ghost commented Dec 19, 2017

What is the release mode? We already have the dev mode, I think it should be checked for equality only in dev mode.

It's when you pass -p <pkg> essentially. We can add an option --promote xxx and -p <pkg> will imply --promote ignore.

May I just raise the point that the current hard-coded formulation of --dev makes it unsuitable in various contexts. So we end up doing "development" builds without --dev, and it would be unfortunate if this new functionality was also unusable as a result.

Absolutely, in the future, I'd like to add profiles. We'll have the release and dev profiles by default and the user will be able to define new ones.

Profiles should be selectable from the command line or from the jbuild-workspace file. Different profiles will enable different defaults, and the user will be able to change the default in a given scope:

(set_defaults
 (dev      ((flags (:standard -w +a)))
 (coverage ((preprocess (pps (bisect.ppx))))

@jberdine
Copy link
Contributor

Adding profiles sounds excellent! I imagine that it would allow using sexp jbuild files in cases like this.

@ghost
Copy link
Author

ghost commented Dec 19, 2017

Indeed, it will. I did notice this jbuild file yesterday when I was trying a patched compiler on a big self-contained workspace, I got unknown context: default...

@jberdine
Copy link
Contributor

That happens when the --root=src option is not passed to jbuilder, see the Makefile. I suspect that I have not fully understood the intended project layout and that this is just an artifact of that.

@ghost
Copy link
Author

ghost commented Dec 19, 2017

Ah I see. The general idea is that by dropping multiple jbuilder projects in any layout you get a new bigger project, that we call a workspace in jbuilder. The root of the workspace is marked by a file jbuild-workspace. So projects might put a jbuild-workspace file at the root, and when a developer creates a big workspace composed of multiple projects, they will create a jbuild-workspace file at the root of the workspace.

Ideally individual jbuilder projects shouldn't rely too much on specific jbuilder invocation. --root is only intended for releases, to prevent jbuilder from scanning the file system on the user machine and finding the wrong root by accident. I imagine that at the moment the lack of profiles and defaults is why we need specific invocations. In the opam file, it is recommended that jbuilder projects always use this build command: ["jbuilder" "build" "-p" name "-j" jobs].

BTW, I did a small video tutorial to explain the basics of jbuilder: https://www.youtube.com/watch?v=BNZhmMAJarw. I'm planning to do a second video to explain the packaging part.

@ghost
Copy link
Author

ghost commented Dec 20, 2017

BTW, I agree that the current project layout is not clear. I guess it's because of the way jbuilder was developed.

When we rename jbuilder, we'll make it clearer: projects will be expected to have a jbuild file (or whatever the new name will be for these files) at the root of their project containing a project stanza.

@ghost
Copy link
Author

ghost commented Jan 11, 2018

Closing this as this is superseded by #402. The only thing missing the the toplevel promote stanza. To implement it we'll need to add a way to extend the curren goal,.

@ghost ghost closed this Jan 11, 2018
@ghost ghost deleted the inline branch February 7, 2018 14:48
This pull request was closed.
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.

4 participants