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

Force regenerate build.ninja #6877

Merged
merged 7 commits into from
Jul 21, 2024
Merged

Conversation

aspeddro
Copy link
Contributor

@aspeddro aspeddro commented Jul 13, 2024

@aspeddro aspeddro changed the title Test warn-error argument Force regenerate build.ninja Jul 13, 2024
@aspeddro
Copy link
Contributor Author

@cknitt you can test it?

@cknitt
Copy link
Member

cknitt commented Jul 16, 2024

@cknitt you can test it?

Thanks a lot! Will test today.

@cknitt
Copy link
Member

cknitt commented Jul 16, 2024

Tested and works fine for me! 🎉

Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Could you do the PR against master?
(I will later backport it to 11.0-release together with other fixes.)

jscomp/build_tests/in_source/input.js Outdated Show resolved Hide resolved
jscomp/runtime/release.ninja Outdated Show resolved Hide resolved
@aspeddro aspeddro changed the base branch from 11.0_release to master July 17, 2024 12:49
@aspeddro
Copy link
Contributor Author

Base now is master. CHANGELOG updated

@aspeddro aspeddro marked this pull request as ready for review July 17, 2024 13:03
@cknitt
Copy link
Member

cknitt commented Jul 17, 2024

Thanks a lot!

I was asking myself if we should further improve this to keep the existing optimization as far as possible (only regenerate build.ninja when needed). Then we would have to remember what -warn-error was set to the last time the build.ninja was generated and extend Bsb_ninja_check.check accordingly.

Right now it is regenerated on every build, so this means if I run rescript -w it will be regenerated on every change, correct?

@aspeddro
Copy link
Contributor Author

Right now it is regenerated on every build, so this means if I run rescript -w it will be regenerated on every change, correct?

Yes!!

I believe there is an effect on performance. It will run this on every build

https://github.com/rescript-lang/rescript-compiler/blob/e0aac3867f89b2b15cc02c1725dc9600a162c94f/jscomp/bsb/bsb_ninja_regen.ml#L44-L100

Another option is to force a cleanup after using -warn-error, see #6868 (comment)

@cknitt
Copy link
Member

cknitt commented Jul 20, 2024

I was asking myself if we should further improve this to keep the existing optimization as far as possible (only regenerate build.ninja when needed). Then we would have to remember what -warn-error was set to the last time the build.ninja was generated and extend Bsb_ninja_check.check accordingly.

@aspeddro Could you investigate how hard it is to do this?

@aspeddro
Copy link
Contributor Author

aspeddro commented Jul 20, 2024

I saved the information in .bsdeps metadata file. The fourth line indicate the latest -warn-error argument. Is zero (0) when you run without -warn-error argument. It's filled with the value passed in the argument. Then we check if there was a change

12.0.0-alpha.1
/home/pedro/Desktop/projects/rescript-compiler/jscomp/build_tests/build_warn_as_error
0
0
rescript.json	0x1.996370153bbc3p+30
src	0x1.9a4c44902961dp+30
===
/home/pedro/Desktop/projects/rescript-compiler/linux/rescript.exe	0x1.9a6f7d3481b04p+30

When you build with rescript build -warn-error +110

12.0.0-alpha.1
/home/pedro/Desktop/projects/rescript-compiler/jscomp/build_tests/build_warn_as_error
0
+110
rescript.json	0x1.996370153bbc3p+30
src	0x1.9a4c44902961dp+30
===
/home/pedro/Desktop/projects/rescript-compiler/linux/rescript.exe	0x1.9a6f7d3481b04p+30

So:

  • A build with -warn-error +110 don't regenerate build.ninja
  • A build without -warn-error regenerate build.ninja
  • A build with -warn-error +27 regenerate build.ninja

Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Awesome work @aspeddro!

@cknitt cknitt merged commit 1079775 into rescript-lang:master Jul 21, 2024
19 checks passed
cknitt pushed a commit to cknitt/rescript-compiler that referenced this pull request Jul 26, 2024
* force regenerate build.ninja

* fix bin path

* Update CHANGELOG.md

* force regenerate when `warn-error` argument change

* restore `-regen` argument

* regenerate if previous warn error != 0
# Conflicts:
#	CHANGELOG.md
cknitt pushed a commit to cknitt/rescript-compiler that referenced this pull request Jul 26, 2024
* force regenerate build.ninja

* fix bin path

* Update CHANGELOG.md

* force regenerate when `warn-error` argument change

* restore `-regen` argument

* regenerate if previous warn error != 0
# Conflicts:
#	CHANGELOG.md
cknitt pushed a commit that referenced this pull request Jul 26, 2024
* force regenerate build.ninja

* fix bin path

* Update CHANGELOG.md

* force regenerate when `warn-error` argument change

* restore `-regen` argument

* regenerate if previous warn error != 0
# Conflicts:
#	CHANGELOG.md
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.

rescript -warn-error does not work correctly on Linux
2 participants