Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Zenoh: Fix badly behaved build.rs scripts. #885

Closed
wants to merge 2 commits into from
Closed

Zenoh: Fix badly behaved build.rs scripts. #885

wants to merge 2 commits into from

Conversation

chachi
Copy link

@chachi chachi commented Mar 30, 2024

build.rs should only ever modify OUT_DIR. These scripts were modifying local files which caused a git index update which caused them to rebuild due to the git-version appearing out of date.

@chachi
Zenoh: Fix badly behaved build.rs scripts.
270eb87 
* Zenoh: Fix badly behaved build.rs scripts.

build.rs should only ever modify OUT_DIR. These scripts were modifying
local files which caused a git index update which caused them to
rebuild due to the git-version appearing out of date.
@eclipse-zenoh-bot
Copy link
Contributor

@chachi If this pull request contains a bugfix or a new feature, then please consider using Closes #ISSUE-NUMBER syntax to link it to an issue.

@J-Loudet
Copy link
Contributor

J-Loudet commented Apr 2, 2024

Thanks for your contribution @chachi!
This looks good to me, if you could fix the formatting issue (by running and committing the result of cargo fmt) I would then be able to merge.

J-Loudet
J-Loudet requested changes Apr 2, 2024
View reviewed changes
Copy link
Contributor

@J-Loudet J-Loudet left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Please run cargo fmt and commit the changes.

@chachi
Copy link
Author

chachi commented Apr 2, 2024

Fixed!

@J-Loudet
Copy link
Contributor

J-Loudet commented Apr 3, 2024

Hi @chachi, thanks for the fix.
It turns out I did not fully grasp the purpose of these build.rs files, they not only validate the config.json5 but also update the content of config_schema.json5.

This is, as you pointed out, not a recommended practice which led to some internal issues: #897
However I am not sure of the course of action.

@milyin @fuzzypixelz what's your take?

@fuzzypixelz
Copy link
Member

fuzzypixelz commented Apr 3, 2024 •
edited
Loading

Hi @chachi, thanks for the fix. It turns out I did not fully grasp the purpose of these build.rs files, they not only validate the config.json5 but also update the content of config_schema.json5.

This is, as you pointed out, not a recommended practice which led to some internal issues: #897 However I am not sure of the course of action.

@milyin @fuzzypixelz what's your take?

I would either:

  1. Remove the code that read/writes config_schema.json5 (in this PR it's a no-op)
  2. Assert that config_schema.json5 is the same as to config::Config without actually overriding it with the correct value

@chachi
Copy link
Author

chachi commented Apr 4, 2024

No problem. Let me know if I should make any changes to this PR or you'd like to take it from here. Happy to help!

@fuzzypixelz
Copy link
Member

No problem. Let me know if I should make any changes to this PR or you'd like to take it from here. Happy to help!

Yes, please do update this PR if you can.

I suggest going with option (2), failing the build when the config schema file doesn't match the Config structure.

J-Loudet added a commit that referenced this pull request Apr 16, 2024
@J-Loudet
fix: build script should not write outside of OUT_DIR
8109238 
See #885

Instead of writing out the result of the parsing of the schema, after
this change the build of the plugins will simply fail.

* plugins/zenoh-plugin-rest/build.rs:
  - don't write out the result of the parsing of the schema in
    zenoh-plugin-rest/config_schema.json5,
  - if the schema does not match the default config.json5, panic.
* plugins/zenoh-plugin-storage-manager/build.rs:
  - don't write out the result of the parsing of the schema in
    zenoh-plugin-storage-manager/config_schema.json5,
  - if the schema does not match the default config.json5, panic.

Signed-off-by: Julien Loudet <julien.loudet@zettascale.tech>
@J-Loudet J-Loudet mentioned this pull request Apr 16, 2024
Merged
@J-Loudet
Copy link
Contributor

See #939

@J-Loudet J-Loudet closed this Apr 16, 2024
Mallets pushed a commit that referenced this pull request Apr 16, 2024
@J-Loudet
fix: build script should not write outside of OUT_DIR (#939)
3537ee6 
See #885

Instead of writing out the result of the parsing of the schema, after
this change the build of the plugins will simply fail.

* plugins/zenoh-plugin-rest/build.rs:
  - don't write out the result of the parsing of the schema in
    zenoh-plugin-rest/config_schema.json5,
  - if the schema does not match the default config.json5, panic.
* plugins/zenoh-plugin-storage-manager/build.rs:
  - don't write out the result of the parsing of the schema in
    zenoh-plugin-storage-manager/config_schema.json5,
  - if the schema does not match the default config.json5, panic.

Signed-off-by: Julien Loudet <julien.loudet@zettascale.tech>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@J-Loudet J-Loudet J-Loudet requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chachi @eclipse-zenoh-bot @J-Loudet @fuzzypixelz