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

Apply -bs-g flag via environment variable #3716

Closed
alex35mil opened this issue Aug 1, 2019 · 42 comments
Closed

Apply -bs-g flag via environment variable #3716

alex35mil opened this issue Aug 1, 2019 · 42 comments
Labels
stale Old issues that went stale

Comments

@alex35mil
Copy link
Contributor

As per Discord discussion, to avoid accidental production builds with -bs-g applied via config, apply it via environment variable instead.

@bobzhang
Copy link
Member

bobzhang commented Aug 3, 2019

Do you use debug build by default, how about generating .g.js for -bs-g?

@alex35mil
Copy link
Contributor Author

alex35mil commented Aug 16, 2019

Do you use debug build by default?

Currently, I turn it on occasionally, when I need to inspect something at runtime, so not by default.

how about generating .g.js for -bs-g

Doesn't it introduce the same issue that it's required to change sources to switch the build? (e.g. change imports in js files)

@TheSpyder
Copy link
Contributor

I would like this (or something similar) as well. I'm starting a new project and hoping to enable -bs-g by default in the early stages, but eventually we'll need a production output mode and it would be a shame to need bsconfig.json adjustments to toggle it.

How about a command line argument to bsb that turns on the -bs-g compiler flag? That would also allow it to be used on demand or (in my case) added to a "dev build" command.

@alex35mil
Copy link
Contributor Author

@TheSpyder Just fyi, flag was the first thing proposed in Discord discussion but @bobzhang preferred env var way back then.

@TheSpyder
Copy link
Contributor

I can still vote for a flag as my preference 😉

An environment var would be fine, though, I'd just set it temporarily e.g. BS_DEBUG=true bsb instead of bsb -debug

@ozanmakes
Copy link

ozanmakes commented Aug 27, 2019

I find bs-g very useful and always have it on in my bsconfig.json. I strip it away using sed for production builds.

If this is going to be implemented, I think it would be useful to have an environment variable similar to "DUNE_PROFILE" instead of one that only turns dev mode on.

@TheSpyder
Copy link
Contributor

strip it away using sed for production builds

Exactly. I don't mind how it's implemented, as long as I don't have to patch the config in a build script.

@erykpiast
Copy link

Anything would be better than sed-ing the config for production build. Who is gonna make the decision here? @bobzhang?

@TheSpyder
Copy link
Contributor

I'm reaching the point where I just want a way to specify a completely different bsconfig.json for my production builds

@davesnx
Copy link
Contributor

davesnx commented Apr 30, 2020

I'm happy to pick up this, do we agreed on it?

Seems like having 2 different bsconfigs it's a pretty common scenario, rather than one env variable that toggles the bs-g. But both are compatible, so, can I just try to create a draft of this?

@bobzhang
Copy link
Member

bobzhang commented May 1, 2020

On vacation, will have a look later

@bobzhang
Copy link
Member

bobzhang commented May 7, 2020

how about using bsb -dev and pickup bsconfig.dev.json, would it complicate things too much?

@TheSpyder
Copy link
Contributor

TheSpyder commented May 7, 2020

@bobzhang that would be awesome!

For bonus points, it would be amazing if bsconfig.dev.json could be a partial config and only overwrite options it specifies (falling back to bsconfig.json. That can be tricky with nested objects, but even a simple shallow overwrite setup would be better than requiring near-complete duplication of bsconfig.json.

IMO that would make it less complicated to understand as well.

@alex35mil
Copy link
Contributor Author

alex35mil commented May 7, 2020

+ for shallow overwrite

@Coobaha
Copy link
Contributor

Coobaha commented May 7, 2020

@bobzhang that would be awesome!

For bonus points, it would be amazing if bsconfig.dev.json could be a partial config and only overwrite options it specifies (falling back to bsconfig.json. That can be tricky with nested objects, but even a simple shallow overwrite setup would be better than requiring near-complete duplication of bsconfig.json.

IMO that would make it less complicated to understand as well.

Also would be super amazing to have bsconfig.prod.json and any other bsconfig.ENV.json that uses bsconfig.json as a base config

@TheSpyder
Copy link
Contributor

Also would be super amazing to have bsconfig.prod.json and any other bsconfig.ENV.json that uses bsconfig.json as a base config

For complicated needs, perhaps, but one step at a time. I'd rather see --dev and bsconfig.dev.json now than wait until bob has time to work on a fully configurable solution.

@yawaramin
Copy link
Contributor

@bobzhang to avoid possible conflicts, I think something like bsb -config dev to pick up bsconfig.dev.json.

@bobzhang
Copy link
Member

bobzhang commented Jun 3, 2020

except -bs-g, do we have more use cases?
Note -bs-g is a bit different, for this flag, when you turn it on, in general you would like to make it effective in your dependency as well.
This is related to ppx-dev-flags #3761, however, it is slightly different, for dependencies, you don't want it to be triggered

@alex35mil
Copy link
Contributor Author

except -bs-g, do we have more use cases?

Different warnings configuration for dev and prod builds.

@bobzhang
Copy link
Member

bobzhang commented Jun 3, 2020

-- sorry if you get double posts

Different warnings configuration for dev and prod builds.

This is actually not suggested, why do you want that?
Note you can turn off some warnings for each file for convenience

@alex35mil
Copy link
Contributor Author

This is actually not suggested, why do you want that?
Note you can turn off some warnings for each file for convenience

In development, having some warnings as warnings but in production turning them into hard errors. Like if there is a big match and I want to try one branch at runtime w/o implementing/mocking others. I don't like adding annotation in this case b/c it is easy to forget to remove one.

@TheSpyder
Copy link
Contributor

TheSpyder commented Jun 3, 2020

I'm sure I've requested different warnings for dev and prod 🤔 maybe that was just a Discord discussion, sorry for not recording it here. I would very much like to change all of my configured warnings to errors in production builds.

I'm also planning to work on some conditional compilation to remove debug code from production; that will involve using -bs-D to turn debug on in development.

The PPX dev adjustments you mentioned is something I'd like to look into in the future, but have never considered due to the difficulty of toggling command line arguments. For example ppx-expect, which will compile tests written inside production source code for dev, but produce no code for those tests in production (or at least that's how I think it works, I've never really tried it).

@yawaramin
Copy link
Contributor

@alexfedoseev I think it was suggested in the Discord that OCAMLRUNPARAM can be used to set the warnings on each run of bsb.

Anyway, if we are indeed going for a generalized bsconfig.[ENV].json approach, where the [ENV] could be passed as a command-line bsb option, it would solve a lot of issues.

@joprice
Copy link

joprice commented Jun 3, 2020

I have the same use case as @TheSpyder, where I want to vary -bs-D flags somehow. Interpolating env vars in bsconfig.json could be another solution to that particular problem.

@alex35mil
Copy link
Contributor Author

@yawaramin Yeah, this is how I address this case currently. But since this issue is being discussed I'd prefer doing everything via configs rather than pretty cryptic OCAMLRUNPARAM.

@bobzhang
Copy link
Member

bobzhang commented Jun 4, 2020

@TheSpyder -bs-D seems to be a valid use case, I am wondering that -bs-g automatically brinings -bs-D DEBUG in scope, would that cover most of your use cases?

@TheSpyder
Copy link
Contributor

@bobzhang that would be fine, yes

@TheSpyder
Copy link
Contributor

I'm only just getting into the conditional compilation I was talking about back in June, and it looks like -bs-g now sets -bs-D DEBUG? And it has done so for a while, since I'm still on v8.0.3?

@TheSpyder
Copy link
Contributor

This worked perfectly, now I just need a way to control -bs-g without editing bsconfig.json so I can remove it in CI :)

@nsculli7
Copy link

nsculli7 commented Dec 2, 2020

-- sorry if you get double posts

Different warnings configuration for dev and prod builds.

This is actually not suggested, why do you want that?
Note you can turn off some warnings for each file for convenience

How would one add an annotation to disable a specific error for a specific file?

The following:

[@bs.config { warnings: { error: "+A-3-44-102-103" } } ]

Produces:
Screen Shot 2020-12-02 at 2 35 03 PM

@alex35mil
Copy link
Contributor Author

@nsculli7 it’s @warning iirc

@nsculli7
Copy link

nsculli7 commented Dec 4, 2020

[@warning "-4"]

Doesn't seem to have any effect.

@alex35mil
Copy link
Contributor Author

@nsculli7 It should work. I used exactly this attribute to workaround compiler issue. Can you post a full snippet?

@farukg
Copy link

farukg commented Apr 1, 2021

if anyone else needs a quick temporary solution using basic nodejs & npm package dotenv to use .env:

  1. rename your bsconfig.json to bsconfig.js
  2. open & edit bsconfig.js as noted in the comments:
// PREPEND  following 2 lines
require('dotenv').config() // + skip this line  if you dont use .env
const bsconfig =  //  makes this a syntactically correct js file
{  // ... FORMER bsconfig.json CONTENT
   // ... FORMER bsconfig.json CONTENT
}  // ... FORMER bsconfig.json CONTENT
// APPEND following lines
const fs = require('fs') 
fs.writeFileSync(
  process.cwd() + '/bsconfig.json',
  '// This file was generated. Please edit bsconfig.js instead\n' + 
    JSON.stringify(bsconfig, null, 2)
)
  1. save & let it format with prettier
  2. include env vars in bsconfig.js
  "bsc-flags": [
    "-bs-D CUSTOM_VAR=" + process.env.CUSTOM_VAR,
  ]
  1. run CUSTOM_VAR="damn, so custom" node bsconfig.js any time your bsconfig.js or your env changes or to be safe always before every build
  2. your done, in this example you'll get:
// This file was generated. Please edit bsconfig.js instead
{
  "bsc-flags": [
    "-bs-D CUSTOM_VAR=damn, so custom"
  ]
}

very basic, better than nothing & done in 5 minutes :)

@mununki
Copy link
Member

mununki commented Oct 18, 2021

Any progress on this? I'd like to build my project with different bsc-flags in development and production mode respectively. It happens a lot to forget to remove the bs-g flag in bsconfig.json accidentally.

@stale
Copy link

stale bot commented Oct 15, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Old issues that went stale label Oct 15, 2023
@TheSpyder
Copy link
Contributor

Please keep this open

@yawaramin
Copy link
Contributor

Maybe can be done at the command line? In local dev e.g. we can set an environment variable RESCRIPT_DEBUG=-bs-g. In CI builds we can just not set it. Then this command works either way:

$ rescript $RESCRIPT_DEBUG build

@stale stale bot removed the stale Old issues that went stale label Oct 16, 2023
@TheSpyder
Copy link
Contributor

TheSpyder commented Oct 17, 2023

@yawaramin -bs-g is not a rescript build option, it's a compiler option. We currently have to set it via bsc-flags in the config.

If it was a build option, then yes there are many easy ways to set it dynamically :) until then I'm still using sed on my config before production builds.

@yawaramin
Copy link
Contributor

Whoops, you're right of course. If the file is kept as plain JSON i.e. without comments I suppose we can also use jq, e.g. in the build step we can do jq '."bsc-flags" |= . - ["-bs-g"]' <bsconfig.json >bsconfig.json to remove the flag in CI.

@TheSpyder
Copy link
Contributor

There are no comments anymore, the vscode extension forces bsconfig.json to JSON instead of JSONC

sed is much easier than jq 🤣 although admittedly that's because my JSON formatter made it easy

config:

  "bsc-flags": [
    "-bs-g"
  ],

sed:
s/"-bs-g"//

Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Old issues that went stale label Oct 17, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Old issues that went stale
Projects
None yet
Development

No branches or pull requests