Skip to content

Check in generated struct info rather than auto-generating it in the sysroot. NFC#19013

Merged
sbc100 merged 1 commit into
mainfrom
remove_auto_struct_info
Mar 21, 2023
Merged

Check in generated struct info rather than auto-generating it in the sysroot. NFC#19013
sbc100 merged 1 commit into
mainfrom
remove_auto_struct_info

Conversation

@sbc100
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 commented Mar 20, 2023

I noticed while working gen_sig_info.py in #18985 that is was simpler to just keep a copy of the generated file checked into source control. We can use a unit test check that its up-to-date on each commit. In fact we already had a unit test that was doing this and keeping a copy of the expected output in the test directory.

This simplifies the emcc linker code since we can always assume the presence of the struct info file.

@sbc100 sbc100 force-pushed the remove_auto_struct_info branch 2 times, most recently from df8c126 to 1e00330 Compare March 20, 2023 16:34
@sbc100 sbc100 requested review from dschuff and kripken March 20, 2023 16:35
@sbc100 sbc100 changed the title Store generated struct info rather than auto-generating it in the sysroot. NFC Check in generated struct info rather than auto-generating it in the sysroot. NFC Mar 20, 2023
@sbc100 sbc100 force-pushed the remove_auto_struct_info branch from 1e00330 to 16cc723 Compare March 20, 2023 19:46
@kripken
Copy link
Copy Markdown
Member

kripken commented Mar 20, 2023

IIUC this means that we need to manually run that gen_struct_info script when changes are needed in that file, is that right? (Compared to before where it was automatic, at least if the cache was cleared.) If so that might be worth documenting in docs/ and/or https://emscripten.org/docs/contributing/developers_guide.html ?

@kripken
Copy link
Copy Markdown
Member

kripken commented Mar 20, 2023

In general this sounds good, but I have the feeling we didn't do it this way for a reason that I can't remember...

@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented Mar 20, 2023

IIUC this means that we need to manually run that gen_struct_info script when changes are needed in that file, is that right? (Compared to before where it was automatic, at least if the cache was cleared.) If so that might be worth documenting in docs/ and/or https://emscripten.org/docs/contributing/developers_guide.html ?

Right, although for local development you still had to remember to do something like ./emcc --clear-cache.. since we didn't have any kind of dependency system that would know when re-generation was needed.

I'll update the docs.

@sbc100 sbc100 force-pushed the remove_auto_struct_info branch 2 times, most recently from 2a2b9ef to f8e5054 Compare March 20, 2023 22:34
Comment thread site/source/docs/contributing/developers_guide.rst Outdated
Comment thread test/test_other.py Outdated
Comment thread test/test_other.py
Comment thread tools/gen_struct_info.py
@sbc100 sbc100 force-pushed the remove_auto_struct_info branch 2 times, most recently from e592dae to ce5658f Compare March 20, 2023 23:02
…root. NFC

I noticed while working `gen_sig_info.py` in #18985 that is was simpler
to just keep a copy of the generated file checked into source control.
We can use a unit test check that its up-to-date on each commit.  In
fact we already had a unit test that was doing this and keeping a copy
of the expected output in the test directory.

This simplifies the emcc linker code since we can always assume the
presence of the struct info file.

Also allows us to remove the `varies=False` special case in cache logic.
@sbc100 sbc100 force-pushed the remove_auto_struct_info branch from ce5658f to 854f498 Compare March 20, 2023 23:04
@sbc100 sbc100 enabled auto-merge (squash) March 20, 2023 23:47
@sbc100 sbc100 merged commit 7e5ab40 into main Mar 21, 2023
@sbc100 sbc100 deleted the remove_auto_struct_info branch March 21, 2023 00:51
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.

2 participants