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

Fix .bs.js timestamp being reset to 1970-01-01 when there are warnings #5738

Merged
merged 1 commit into from Oct 16, 2022

Conversation

cknitt
Copy link
Member

@cknitt cknitt commented Oct 16, 2022

Closes #5206.

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Tested and it works fine for me.
Anything else to test here? Some bigger project or something.

Other than that, please add a line in the CHANGELOG.

@cknitt
Copy link
Member Author

cknitt commented Oct 16, 2022

I already tested with a fairly large project.

Rebased and added to CHANGELOG.

@cknitt cknitt merged commit dca052b into rescript-lang:10.1_release Oct 16, 2022
@cknitt cknitt deleted the fix-timestamp-reset branch October 16, 2022 12:09
@bobzhang
Copy link
Member

bobzhang commented Oct 18, 2022

@cknitt sorry for the late comments, note the fix was correct but inefficient.

Set the .mlast as old, would trigger its dependencies rebuild while nothing changes, what we want is only files with warnings rebuilt, so if you have warnings, the rebuild would take a while.

The proposed fix was tracking cmt[i] file in the generated ninja file, we set the cmt[i] file as old.
Why cmt file? It is a leaf node in the build graph that is not used by any other rule, so only the current artifact is rebuilt.

maybe some kind of additional "marker" file should be created for files with warnings?

cmt[i] file would be a good candidate

cc @cristianoc

@cknitt
Copy link
Member Author

cknitt commented Oct 18, 2022

@bobzhang Thanks for the hint, I will have another look at this.

@bobzhang
Copy link
Member

So you can split into two steps:

  • Change the ninja generator to track cmt/cmti files -- no side effects, no regressions
  • set the cmt/cmti as old file

@cknitt
Copy link
Member Author

cknitt commented Oct 30, 2022

@bobzhang @cristianoc I have retested the current implementation that sets the timestamp of the .ast file and did not see any dependencies being unnecessarily recompiled.

For example, I have a warning in AuthManager.res and have this in the ninja file:

o src/auth/AuthManager.ast : astj ../../src/auth/AuthManager.res
o src/auth/AuthManager.d : deps src/auth/AuthManager.ast src/auth/AuthManager.iast
o src/auth/AuthManager.cmj ../../src/auth/AuthManager.bs.mjs : mj src/auth/AuthManager.ast src/auth/AuthManager.cmi

Now when I run rescript build again, I can see that AuthManager.res is recompiled.

So IMHO the feature is working fine and there is no inefficiency / performance regression.

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.

None yet

3 participants