Bytecode header tests + normalization #4516
Conversation
…urrent build generates matches with the current header file.
|
Looks like there's some issue with using the regex header from ch on xplat. We also discussed issues with the source file (e.g. Intl.js) having different line endings since that isn't specified by gitattributes at the moment, did you come up with a solution for that? |
bin/ch/ch.cpp
Outdated
| #include "stdafx.h" | ||
| #include "Core/AtomLockGuids.h" | ||
| #include <CommonPal.h> | ||
| #include <regex> |
There was a problem hiding this comment.
Can we not add dependency to regex and fix the line ending instead?
There was a problem hiding this comment.
One problem it might cause is that the character offset for line/col numbers in the generated byte code will be different then JS file on disk
There was a problem hiding this comment.
I believe @tcare 's investigation into line endings with gitattributes suggests that the strongest defense we have is core.autocrlf, which we can't enforce. When we regen bytecode, could we also re-write the file we are generating bytecode after normalization back to disk?
There was a problem hiding this comment.
I don't think modifying the source JS files is a great idea, since this process isn't just used for code that we own, it's also used on WWAs (and in particular I think on WWAs that are not on the developer's machine, but on the end-user's machine)
There was a problem hiding this comment.
This is only intended to be for internal library js files. WWA shouldn't hit this path?
There was a problem hiding this comment.
One problem it might cause is that the character offset for line/col numbers in the generated byte code will be different then JS file on disk
This is exactly the problem this is trying to solve. Since we can't reliably enforce line endings on the javascript files from source control. It is simpler to normalize the line endings so we get the same line offset wherever the header is generated.
When we regen bytecode, could we also re-write the file we are generating bytecode after normalization back to disk?
The line endings of the file is not actually wrong, it just depends on the setting on one's computer.
I don't think modifying the source JS files is a great idea, since this process isn't just used for code that we own, it's also used on WWAs (and in particular I think on WWAs that are not on the developer's machine, but on the end-user's machine)
This change is localized to our testing host, therefore, it doesn't affect WWA.
I will try with the solution I used in ChakraFull using strtok instead.
After talking to @tcare it seems we don't have a reliable way to ensure those javascript files will always have the right line endings. Normalization upon consumption seemed the most reliable solution. |
…arison. Use strtok instead of regex to normalize files being serialized.
Merge pull request #4516 from Cellule:bytecodeheader_test Add unit test to regenerate bytecode header to verify that what the current build generates matches with the current header file. Normalize line endings before serializing bytecode header for library code. ChakraFull PR: https://devdiv.visualstudio.com/DevDiv/_git/Chakra/pullrequest/100648?_a=overview
Merge pull request #4516 from Cellule:bytecodeheader_test Add unit test to regenerate bytecode header to verify that what the current build generates matches with the current header file. Normalize line endings before serializing bytecode header for library code. ChakraFull PR: https://devdiv.visualstudio.com/DevDiv/_git/Chakra/pullrequest/100648?_a=overview # Conflicts: # lib/Runtime/Library/InJavascript/Intl.js.bc.32b.h # lib/Runtime/Library/InJavascript/Intl.js.bc.64b.h # lib/Runtime/Library/InJavascript/Intl.js.nojit.bc.32b.h # lib/Runtime/Library/InJavascript/Intl.js.nojit.bc.64b.h
Add unit test to regenerate bytecode header to verify that what the current build generates matches with the current header file.
Normalize line endings before serializing bytecode header for library code.
ChakraFull PR: https://devdiv.visualstudio.com/DevDiv/_git/Chakra/pullrequest/100648?_a=overview
This change is