Skip to content

Modular Jsoo settings [WIP]#1044

Closed
rgrinberg wants to merge 3 commits intoocaml:mainfrom
rgrinberg:jsoo-separate-compilation
Closed

Modular Jsoo settings [WIP]#1044
rgrinberg wants to merge 3 commits intoocaml:mainfrom
rgrinberg:jsoo-separate-compilation

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg commented Jul 22, 2018

WIP to address #970

I've added the basic functionality that --dev enabled via the single jsoo_compilation field. However, the old mode is a bit more involved than just separate compilation and also involved some flag switching. The flags thing I think is best handled the same way we handle ocaml_flags. So either include the profile dependant jsoo flags in Ocaml_flags.t or make something similar.

I'm also taking suggestions for naming. What I think would be ideal if we had a field for env that looked like this:

(js_of_ocaml
 (compilation separate|classic)
 (flags <osl>))

Btw: js_of_ocaml is kind of a mouthful, I'd be in favor of deprecating it and adopting jsoo instead in the dune lang.

rgrinberg added 2 commits July 23, 2018 00:28
@rgrinberg
Add jsoo_compilation option to env
eabf67b 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
Do the minimum work to make the old --dev for jsoo dir scoped
15cf879 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg rgrinberg requested a review from a user July 22, 2018 22:35
hhugo
hhugo reviewed Jul 23, 2018
View reviewed changes
src/js_of_ocaml_rules.ml Outdated
let separate_compilation_enabled = dev_mode
let pretty sctx ~dir =
match jsoo_compilation sctx ~dir with
| Separate -> ["--pretty"]
Copy link
Copy Markdown
Collaborator

@hhugo hhugo Jul 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

--pretty and --sourcemap have no relationship with separate compilation. They should be enabled with the dev profile.

Copy link
Copy Markdown
Member Author

@rgrinberg rgrinberg Jul 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yup that is true. I just kept the old behavior to keep things simple for an initial PR. As I said in my ticket, the plan is to treat these as normal default flags for dev profile

@rgrinberg
Copy link
Copy Markdown
Member Author

rgrinberg commented Jul 23, 2018

@diml when you're back, can you add hhugo to the team of maintainers as the jsoo maintainer? I could not select him from the list of reviewers.

@hhugo What do you think about splitting the jsoo flags field into links and compile flags in analogy to the regular compile flags. This way we'll be able to set --source-map-inline only to jsoo_link and then we'll be able to have proper profile dependant defaults.

@rgrinberg
WIP jsoo
796c9bc 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
Copy link
Copy Markdown
Member Author

rgrinberg commented Jul 27, 2018

Depends on #1048

@hhugo
Copy link
Copy Markdown
Collaborator

hhugo commented Jul 30, 2018

@rgrinberg, --source-map-inline is also needed when compiling.

@rgrinberg
Copy link
Copy Markdown
Member Author

rgrinberg commented Jul 30, 2018

Understood, but we still need separate fields for compilation and linking. Otherwise there won't be a way to set the defaults such that --pretty isn't passed to jsoo_link.

@hhugo
Copy link
Copy Markdown
Collaborator

hhugo commented Jul 30, 2018 •
edited
Loading

Well, it's a bit more complicated.

  • js_of_ocaml (the binary) can do both "compilation" and "linking".
  • jsoo_link is only use to link js files together in separate compilation.
  • some jsoo_link flags cannot be pass to js_of_ocaml

@ghost
Copy link
Copy Markdown

ghost commented Jul 30, 2018

I added @hhugo

@rgrinberg
Copy link
Copy Markdown
Member Author

rgrinberg commented Jul 31, 2018

Thanks for clarifying @hhugo, this does complicate things a little bit. Essentially, the link flags would be entirely useless separate compilation was turned on. I see possibilities such as:

  • Making it clear to the user that link flags are actually jsoo_link flags with a jsoo_link flag. But I don't like this, because it's a bit too low level of a detail for users.

  • adding a separate compilation flags field such as:

(separate_compilation_flags
 (compile <flags>)
 (link <flags))

These flags would only be consumed in separate compilation mode of course.

  • Sticking to my original proposal but simply ignoring the link flags when separate compilation is off. The only issue here is that a user might not expect the link flags to be ignored but it's not so bad.

Any other ideas you have in mind? Or a preference to any of the above?

@rgrinberg
Copy link
Copy Markdown
Member Author

rgrinberg commented Sep 1, 2018

ping @hhugo. Not urgent, but I would like this in for 1.3

@rgrinberg rgrinberg mentioned this pull request Dec 6, 2018
Closed
@pbiggar
Copy link
Copy Markdown

pbiggar commented Dec 6, 2018

@rgrinberg asked me to comment on this proposal as I have a related request in #1613.

I think it's good. I tried to use this functionality without realizing it didn't exist, and the thing I tried looks a lot like this.

The one comment I have is on the naming of the compilation flag. I think classic doesn't mean anything. I would rename it. Perhaps an antonym to "separate" such as "together" or "single", or a term from traditional compilation like "whole-program". Hope that's not too bikeshedy.

@rgrinberg rgrinberg marked this pull request as draft April 29, 2020 15:06
Base automatically changed from master to main January 14, 2021 17:08
@Lupus
Copy link
Copy Markdown

Lupus commented Sep 22, 2021

Has this been considered a go or no go? Setting --enable=with-js-error --enable=excwrap globally is still not possible without pinning js_of_ocaml with this option changed in-code...

@hhugo hhugo mentioned this pull request Oct 25, 2021
Merged
9 tasks
@hhugo
Copy link
Copy Markdown
Collaborator

hhugo commented Oct 25, 2021

I'm looking at this now.
Will work on #5049

@rgrinberg rgrinberg added the jsoo label Nov 4, 2021
@rgrinberg
Copy link
Copy Markdown
Member Author

rgrinberg commented Nov 4, 2021

Superseded by #5049

@rgrinberg rgrinberg closed this Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@hhugo hhugo hhugo left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

jsoo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rgrinberg @hhugo @pbiggar @Lupus