Skip to content

Conversation

jschwe
Copy link
Member

@jschwe jschwe commented Jul 24, 2025

Using vendored rust crates causes problems with cargo vendor and offline builds as well as cargo publish.
Using cargo metadata is currently the officially recommended solution to get the location of the c header files, but ideally we add a patch upstream to support reading the location from a DEP_ environment variable, that cargo could set for us (posted unicode-org/icu4x#6769).

For ease of reviewing the changes to the build system and removing the now unnecessary vendored code are separate commits.

@jschwe jschwe force-pushed the jschwender/icu_capi_testing branch from 35fb8f0 to fc81aeb Compare July 24, 2025 11:35
@jschwe jschwe force-pushed the jschwender/icu_capi_testing branch from fc81aeb to 72a603d Compare August 8, 2025 13:55
@jschwe jschwe mentioned this pull request Aug 8, 2025
jschwe added 2 commits August 20, 2025 11:54
Use the headers provided by cargo, instead of our local checkout.
This also revealed that we had been using version 1.5, when
we wanted to use 1.4, to be in sync with upstream.

Signed-off-by: Jonathan Schwender <[email protected]>
Signed-off-by: Jonathan Schwender <[email protected]>
@jschwe jschwe force-pushed the jschwender/icu_capi_testing branch from 72a603d to 84996cb Compare August 20, 2025 09:54
Signed-off-by: Jonathan Schwender <[email protected]>
@jschwe
Copy link
Member Author

jschwe commented Aug 20, 2025

@sagudev @jdm Does either of you know if I can set includes in moz.build, that point to somewhere in $CARGO_HOME? Currently I'm patching out the LOCAL_INCLUDES line, and setting -I<path> via CXXFLAGS, but that doesn't seem to work on windows (probably I need to escape the \ component in the path or similar). But I guess it would be nicer to use moz.build instead, so I'm wondering if there is a way to set INCLUDES to an arbitrary path.

@sagudev
Copy link
Member

sagudev commented Aug 20, 2025

I think you can use env vars in moz.build.

EDIT: Or maybe not and you need to write wrapper to get it available in https://firefox-source-docs.mozilla.org/build/buildsystem/mozbuild-symbols.html#config

Signed-off-by: Jonathan Schwender <[email protected]>
@jschwe
Copy link
Member Author

jschwe commented Aug 20, 2025

I think you can use env vars in moz.build.

I think that part worked (but now I'm not sure anymore). What caused me problems is that LOCAL_INCLUDES apparently doesn't accept include paths from outside the mozjs root.

@jschwe jschwe marked this pull request as ready for review August 20, 2025 11:47
@jschwe jschwe changed the title Draft: Use cargo provided icu_capi Use cargo provided icu_capi Aug 20, 2025
@jschwe
Copy link
Member Author

jschwe commented Aug 20, 2025

I believe this PR would also allow publishing mozjs via crates.io (unless there were other blockers besides the vendored rust crates)

@jschwe jschwe added this pull request to the merge queue Aug 20, 2025
Merged via the queue into servo:main with commit af903e0 Aug 20, 2025
34 checks passed
github-merge-queue bot pushed a commit to servo/servo that referenced this pull request Aug 21, 2025
Instead of vendoring a copy of icu_capi, mozjs now instead determines
the location of the provided c header files by parsing the cargo
metadata output.
This will allow vendoring mozjs and is a step towards publishing mozjs
and thus servo again.
Corresponding mozjs PR: servo/mozjs#596

Testing: Covered by existing tests

Signed-off-by: Jonathan Schwender <[email protected]>
github-merge-queue bot pushed a commit to servo/servo that referenced this pull request Aug 21, 2025
Instead of vendoring a copy of icu_capi, mozjs now instead determines
the location of the provided c header files by parsing the cargo
metadata output.
This will allow vendoring mozjs and is a step towards publishing mozjs
and thus servo again.
Corresponding mozjs PR: servo/mozjs#596

Testing: Covered by existing tests

Signed-off-by: Jonathan Schwender <[email protected]>
github-merge-queue bot pushed a commit to servo/servo that referenced this pull request Aug 21, 2025
Instead of vendoring a copy of icu_capi, mozjs now instead determines
the location of the provided c header files by parsing the cargo
metadata output.
This will allow vendoring mozjs and is a step towards publishing mozjs
and thus servo again.
Corresponding mozjs PR: servo/mozjs#596

Testing: Covered by existing tests

Signed-off-by: Jonathan Schwender <[email protected]>
github-merge-queue bot pushed a commit to servo/servo that referenced this pull request Aug 21, 2025
Instead of vendoring a copy of icu_capi, mozjs now instead determines
the location of the provided c header files by parsing the cargo
metadata output.
This will allow vendoring mozjs and is a step towards publishing mozjs
and thus servo again.
Corresponding mozjs PR: servo/mozjs#596

Testing: Covered by existing tests

Signed-off-by: Jonathan Schwender <[email protected]>
github-merge-queue bot pushed a commit to servo/servo that referenced this pull request Aug 21, 2025
Instead of vendoring a copy of icu_capi, mozjs now instead determines
the location of the provided c header files by parsing the cargo
metadata output.
This will allow vendoring mozjs and is a step towards publishing mozjs
and thus servo again.
Corresponding mozjs PR: servo/mozjs#596

Testing: Covered by existing tests

Signed-off-by: Jonathan Schwender <[email protected]>
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