Skip to content

remove special support for mll files #1664

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

Closed
wants to merge 5 commits into from
Closed

remove special support for mll files #1664

wants to merge 5 commits into from

Conversation

bobzhang
Copy link
Member

@bobzhang bobzhang commented May 31, 2017

TODO: we need ignore generated files in watch mode.

@TheSpyder
Copy link
Contributor

I strongly disagree with having the generator output into the source tree. Other build systems don't do this, it's a change from how bsb handled mll files previously, and I don't think checking the generated ML code into my source tree is a good idea (it's about 85kb in my case, changes will severely pollute the diffs).

@bobzhang
Copy link
Member Author

bobzhang commented Jun 1, 2017

hi @TheSpyder , you don't need check in if you don't like add it to .gitignore. But you do need have it when you publish it to npm (so you need add something to .npmignore), would it work for you?

Note "don't check in your generated code" is a widespread but controversial opinion. IIRC, google check in proto-buf generated code, we also check in schema generated code internally. For example, if you use menhir as a code generator but did not check in generated code, then the user would have to pull such huge dependencies just to build a parser while it is unlikely to work on some platforms such as Windows. The design of go generate also mandates user to publish generated code: https://blog.golang.org/generate

I understand there is a tension between library developer happiness and library user happiness, I tend to be in favor of library user happiness

@TheSpyder
Copy link
Contributor

@bobzhang I wouldn't need the .ml file publishing to npm, I'd either publish the final .js or list menhir as a dependency. This is is what I see in the opam ecosystem too, projects with generated code (e.g. reason-parser) have menhir as an install dependency.

I can see an argument for checking in the final .js output, even if I disagree with it. I don't think that extends to the menhir and ocamllex generated .ml files, both old (ocamlbuild) and new (jbuilder) build systems put them in the build area of the tree. As does BSB, before this change.

And while I can gitignore the files, they're hard to ignore in sublime (file ignores are all substring match, so ignoring file.ml also hides file.mly).

@bobzhang
Copy link
Member Author

bobzhang commented Jun 2, 2017

@TheSpyder from what I can tell, ocamlbuild or jbuilder are not well tested on Windows.. Make your library more accessible is really important, also if you check in your generated code, you have more freedom to use whatever code generator you like. BuckleScript compiler internally used camlp4, but users don't need anything about camlp4(or go through the pain of installing camlp4), that's the point.

And while I can gitignore the files, they're hard to ignore in sublime (file ignores are all substring match, so ignoring file.ml also hides file.mly).

I would be surprised if sublime does not support regex based ignore...

@TheSpyder
Copy link
Contributor

Publishing generated code to npm and checking generated code in to source are two very different things, imo. It shouldn't be the job of developers on my team to guarantee the generated code equals the change they made to the original source file - that's what build servers and npm publishing scripts are for. Even checking .js into source seems like something that would only be done by teams migrating from JS -> BS, once the code is 100% generated you'd stop checking the JS in.

Maybe that means people installing from a git URL instead of npm need the generator tool, but I think that's fine. They're likely developers anyway.

Which build tool works on what platform doesn't seem relevant. I'm not really trying to convince you to agree with my position, I just want a choice between whether to check generated code into source or not. IMO putting the files in my source tree (which no other tool does) makes that harder.

As for sublime... ignore is only substring match, not regex. file.ml is a substring of file.mly and file.mll so it hides the source as well as the generated code.

@bobzhang
Copy link
Member Author

bobzhang commented Jun 5, 2017

Publishing generated code to npm and checking generated code in to source are two very different things, imo

Yes, it is up to you to version control generated files or not, what we ask is publish your generated code into npm to make others' life easier, so it is good ?

@TheSpyder
Copy link
Contributor

I agree with what you're saying, but having the generated files in the source tree makes not checking them in to version control harder. I'm probably migrating to visual studio code anyway, and hiding the .ml file works there, but not in sublime.

Having generated files in the source tree just seems like a return to the old Makefile days. Maybe I'm the only one who tried to use the specific .mll support before this change, but I preferred that approach where the generated file was kept separate.

I've been migrating the native target of my project to jbuilder, and it looks like it achieves separation by duplicating the source tree and compiling there. Jbuilder does list windows as a supported platform, btw.

@hackwaly
Copy link

hackwaly commented Jun 8, 2017

The generated files are in source tree, and so bsb -clean doesn't clean those, ~~~and seems bsb -watch doesn't work: when .mly files change, it doesn't generate fresh .ml files~~~.

@TheSpyder
Copy link
Contributor

Watch does seem to work for me, and clean not removing them fits with the stated goal of checking the generated file in to source control. I hadn't noticed that clean leaves them behind, but they're already hidden by my git/vscode configs.

@TheSpyder
Copy link
Contributor

ooh, I didn't notice this before: the generator implementation seems to send bsb -watch into an infinite recompile loop when the tool throws an error and thus doesn't update the target .ml file.

@bobzhang
Copy link
Member Author

bobzhang commented Jun 9, 2017 via email

@TheSpyder
Copy link
Contributor

The weird part about that is watch doesn't go into an infinite loop with menhir errors, but it does with ocamllex.

@TheSpyder
Copy link
Contributor

I reinstalled from this branch today, and it's weird that the branch doesn't seem to have changed since I last installed yet now I'm finding that generators aren't firing in dependencies? I'm sure this worked before.

I can run bsb inside the node_modules dependency folder (after linking bs-platform) and it generates fine, but during a bsb -make-world it throws a file not found rather than generating.

@bobzhang
Copy link
Member Author

i will rebase branch and probably will land it one release after

@bobzhang
Copy link
Member Author

bobzhang commented Jul 3, 2017

hi @TheSpyder I will land this feature in #1758 this week.

I can run bsb inside the node_modules dependency folder (after linking bs-platform) and it generates fine, but during a bsb -make-world it throws a file not found rather than generating.

IIUC, you did not check in generated files. The design of such feature is to publish generated files, so that when it is used as dependencies, it does not need to be generated again

@TheSpyder
Copy link
Contributor

OK I'll give that branch a go 👍

I don't think checking in dependencies will solve the file not found problem. In fact, it might make it worse. Anything compiled with es6-global will have paths relative to the node_modules they're compiled with (i.e. if checked in, that's inside the dependency's node_modules folder).

I know I'm pushing fairly hard at a bit of an edge case, but with browsers starting to roll out ES6 support I really want my project to be developed with just bsb and not require a post-build packaging step :)

@bobzhang
Copy link
Member Author

bobzhang commented Jul 3, 2017

There might be some miscommunications, it is okay to not publish js files, but for meta-programming, like ocamllex, ocamlyacc, it is needed to publish generted files, otherwise users need install ocamllex ocamlyacc.
It is up to user to publish js files or not since bsb knows how to generate those files, but it is needed for customer generator, otherwise the end users need to have those dependencies installed.
the whole point is cut dependencies as early as we can

@TheSpyder
Copy link
Contributor

TheSpyder commented Jul 3, 2017

ah yeah I forgot my own complaint, I think that was a separate issue I was having. In the two weeks since I reported that, the second project is more stable so I don't need to npm link it anymore 🙂

With my dual JS/Native build system, ocaml is already a dependency and it includes ocamllex out of the box. Menhir is similarly easy to require. Sorry if I'm pushing too hard at the edge cases here; I don't mind what you're saying being the default but I don't plan to check in my generated code for private projects.

@TheSpyder
Copy link
Contributor

ah, I figured it out. #1691 is what I was talking about, which is what happens if I set it up so that the generated .ml files are effectively checked in (they're still generated but separate to the bsb process). Maybe I should stop saying generated for the bsb output in context of this bug 🤔

@bobzhang
Copy link
Member Author

already landed

@bobzhang bobzhang closed this Jul 14, 2017
@bobzhang bobzhang deleted the bsb_generate branch July 14, 2017 01:43
@TheSpyder
Copy link
Contributor

This "generate files next to source" thing continues to bite me, btw. I will admit that I'm in something of a unique situation with the problem I had today.

I just spent half an hour struggling to fix a bug in my generated code when it turns out none of the changes I was making were applied because jbuilder was picking up the bsb-generated file instead of generating a new one (it generates into a custom build folder).

@bobzhang
Copy link
Member Author

bobzhang commented Aug 22, 2017 via email

@TheSpyder
Copy link
Contributor

but then how would jbuilder know what to do with it?

@bobzhang
Copy link
Member Author

bobzhang commented Aug 22, 2017 via email

@TheSpyder
Copy link
Contributor

The failure was happening before my build even got to the bsb stage. I run jbuilder first since it's being used to compile tests (until bsb-native stabilises), the error style is subtly different to bsc and that messes up the vscode task monitor. I usually have a step in my build that deletes the generated files before running jbuilder but I accidentally took them out of the loop which also contributed to the issue.

We're going around in circles. I don't mind that you prefer generated code to be checked in. I just wish there was an option to behave like jbuilder / ocamlbuild and not do that.

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