[Android][Hermes][Sourcemaps][Release] Fix for sourcemap-output external configuration breaks bundling

[joe-sam]

Summary

This is a fix for the issue Setting --sourcemap-output in extraPackagerArgs in project.ext.react breaks bundling #32239
This bug only affects Hermes with Android on release builds.
Hermes sourcemaps are generated as part of a multistage process for release builds combining the intermediate hbc compiler and packager maps to produce the final bundle map using the compose-sourcemap scripts.
Overriding the sourcemap-output parameter, breaks the intermediate destination of Hermes packager sourcemaps.
This fix corrects the intermediate/final destination for the final sourcemap-output as specified by the user without changing the intermediate destinations permitting the maps to build without a hitch.
Note that
1 the sourcemap-output build destination must pre-exist or be re-generated as part of the usual bundle process.
2 sourcemaps are only generated for release build variants and not other variants.

Changelog

[ANDROID][FIXED][HERMES][RELEASE][SOURCEMAPS] - Setting --sourcemap-output in extraPackagerArgs in project.ext.react breaks bundling

Test Plan

Create a new project with npx react-native init
Enable Hermes in app/build.gradle
Modify the following configuration section to app/build.gradle

def bundleAssetName  = "test_hermes_with_extraargs.index.android.bundle"
project.ext.react = [
    enableHermes: true,  // clean and rebuild if changing
    // the name of the generated asset file containing your JS bundle
    bundleAssetName: "${bundleAssetName}",
    // supply additional arguments to the packager otherwise use defaults
    extraPackagerArgs: ["--sourcemap-output", "${buildDir}/${bundleAssetName}.map"]
]

run cd android && ./gradlew assemblerelease

Retry with other combinations of enableHermes: false/true and with/without extraPackagerArgs sourcemap-output overrides and check if the source map corresponding to the bundle are generated as expected in the user specified destination override OR in the default build locations when override not specified.

[facebook-github-bot]

Hi @joe-sam!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,776,574	+0
android	hermes	armeabi-v7a	7,181,838	+0
android	hermes	x86	8,085,340	+0
android	hermes	x86_64	8,065,617	+0
android	jsc	arm64-v8a	9,650,041	+0
android	jsc	armeabi-v7a	8,424,052	+0
android	jsc	x86	9,599,444	+0
android	jsc	x86_64	10,196,852	+0

Base commit: 0337a29
Branch: main

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 0337a29
Branch: main

[cortinico]

Hey @joe-sam,
Thanks for the PR.

I'm trying to understand why this is even needed. I would rather allow customizing the sourcemap location rather than doing all the CLI parameter handling you're doing.

[joe-sam]

Sorry for the late response @cortinico my internet went down for more than a few days due to inclement weather.
I would recommend my way for the reasons below.

1. The current approach seems to be trying to get Hermes to be the default engine.
   As such millions of devs like me will be migrating to Hermes with the enableHermes flag set (default is false) to try before opting in. My guess is that 0.68.2 will be one of the last couple of such releases with the flag.

To ensure a smooth and seamless transition I would not introduce a new config option for a minor release such as 0.68.2, as has been done by the patch as millions of devs would miss such a breaking change, until it shows up too late at a maintenance/debug phase on an integration like Sentry or Bugsnag etc. I caught mine more than a month after release after migrating to Hermes and was confounded by the enigmatic stack trace till I stumbled on this issue.
This may be okay to do for a major release such as 0.69 although I would expect exactly the same results, since most devs would not read the docs about config vars and miss that change till too late.

2. I'm not an expert in gradle (fortunately it seems to be vaguely JVM based) and there may be a much cleaner way to swap out those defs, without that horrendous if-else block, however I have tried the complete test plan matrix against it and nudged it until I satisfied myself that it works.
   But it seems really fortunate to me that the CLI parameters were designed to be put in an array in order just to enjoy those benefits such as to allow CLI parameter handling and any de-duplication checks.
   Therefore I have also extended this logic by coalescing all the arguments together into a single argument for any such future eventuality.

edit
3. Finally but most importantly with the aforementioned "patch" on the corresponding issue (#32239) is not implicitly correct as it only tries to alters the intermediate destination of packager map as a fix whereas actually it is the final output source map destination that must be set.

[cortinico]

I caught mine more than a month after release after migrating to Hermes and was confounded by the enigmatic stack trace till I stumbled on this issue.

Thanks for the detailed response @joe-sam, but it still doesn't answer my original point: why is this change even needed in the first place? You should be fine by overriding the sourcemap location instead, rather than manipulating the CLI parameters

[joe-sam]

Thanks for the detailed response @joe-sam, but it still doesn't answer my original point: why is this change even needed in the first place? You should be fine by overriding the sourcemap location instead, rather than manipulating the CLI parameters

Yes that should work perfectly fine as well ,I misunderstood your original question, and is certainly cleaner implementation. I certainly have no issues if you wish to implement it that way. I was trying to avoid adding in new configurations and use one CLI override for extraPackagerArgs which I assumed was standard behaviour. If I have understood you right then instead of the CLI override, you will now require two extra config objects specifying the intermediate and final paths defined in project.ext.react array for example [config.OutputSourceMapFile, config.PackagerSourceMapFile] defined in project.ext.react one for the which can override the defaults.
and then in app/build.gradle

 project.ext.react = [
    enableHermes: true,  // clean and rebuild if changing,
    OutputSourceMapFile: "${buildDir}/${bundleAssetName}.map"
    PackagerSourceMapFile: "${buildDir}/${bundleAssetName}.packager.map"
]

and in react.gradle
def jsPackagerSourceMapFile = config.PackagerSourceMapFile?config.PackagerSourceMapFile:file("$jsIntermediateSourceMapsDir/${bundleAssetName}.packager.map")

def jsOutputSourceMapFile = config.OutputSourceMapFile?config.OutputSourceMapFile:file("$jsSourceMapsDir/${bundleAssetName}.map")

If this is more on the lines of what you meant I can close this and submit a new PR to resolve the issue.

[cortinico]

You should be fine with:

project.ext.react = [
    enableHermes: true,
    bundleAssetName: "mycustomname.bundle"
]

This allows you to customize the bundle name and the sourcemap filename.
It should be sufficient to offer some degree of customization.

Customizing the sourcemap location is not possible at the moment, and I still haven't fully understood what's the use case for it.

[joe-sam]

Alright since there is no use case for custom sourcemap location I will close this pull request.