WIP: Generate native binary from java code #2302

viniciusfcf · 2023-05-29T19:55:31Z

Checkstyle is not ok.

What does this implement/fix? Explain your changes.

Java projects could generate jar or native files to run (Quarkus, Micronaut or SpringBoot). Now it's very easy after accept this PR. Example with Quarkus using this plugin: https://github.com/viniciusfcf/quarkus-azure-functions-http/tree/using-new-versions
…

Does this close any currently open issues?

No, however I'm working with Sandra Ahlgrimm to allow native function using customHandler from java projects

Any relevant logs, screenshots, error output, etc.?

Any other comments?

…

Has this been tested?

Tested

viniciusfcf · 2023-05-29T19:56:59Z

/cc @SandraAhlgrimm

viniciusfcf · 2023-05-29T20:03:28Z

@microsoft-github-policy-service agree

@microsoft-github-policy-service agree company="Red Hat"

edburns · 2023-05-30T17:23:23Z

Converting to Draft. I'll seek guidance from @Flanker32 , but in general I am in favor of this feature.

Flanker32 · 2023-05-31T03:12:24Z

...ions-maven-plugin/src/main/java/com/microsoft/azure/maven/function/AbstractFunctionMojo.java

@@ -323,6 +354,8 @@ public Map<String, String> getTelemetryProperties() {
        result.put(DISABLE_APP_INSIGHTS_KEY, String.valueOf(isDisableAppInsights()));
        final boolean isDeployToFunctionSlot = getDeploymentSlotSetting() != null && StringUtils.isNotEmpty(getDeploymentSlotSetting().getName());
        result.put(FUNCTION_DEPLOY_TO_SLOT_KEY, String.valueOf(isDeployToFunctionSlot));
+        result.put(FUNCTION_IS_NATIVE_EXECUTABLE, String.valueOf(isNativeExecutable()));
+        result.put(FUNCTION_CUSTOM_HANDLER_ARGS, customHandlerArgs);


Will ‘customHandlerArgs’ contains any PII data like file location? If so, we should not record them in telemetry but we could track whether users set custom handler args here

Actually I don't know. Do you suggest to remove both lines?

I prefer to keep line 357, so that we could track how many users use native executable, for custom handler args, I'm not sure whether we need track it, so may be it could be removed

azure-functions-maven-plugin/src/main/java/com/microsoft/azure/maven/function/PackageMojo.java

...e-lib/src/main/java/com/microsoft/azure/toolkit/lib/appservice/config/FunctionAppConfig.java

Flanker32 · 2023-05-31T09:09:23Z

...ions-maven-plugin/src/main/java/com/microsoft/azure/maven/function/AbstractFunctionMojo.java

        if (!result.exists()) {
            throw new AzureToolkitRuntimeException(CAN_NOT_FIND_ARTIFACT);
        }
        return result;
    }

+    protected String finalNameWithExtension() {


getArtifact will be called by compile level validation and the function.json generation logic, we may be careful to modify its implementation. It seems that this method will only be called during function package, so could we just move this method to Package.mojo, and modify copyBinaryToStageDirectoryto get the correct artifact?

Besides, we may add a parameter like nativeExecutablePath' instead of boolean nativeExecutable`, as we could only guess the final artifact here, but users may modify it with configuration in quarkus maven plugin

So, just to validate if I understand all:

Move getArtifact from AbstractFunctionMojo to Package

change nativeExecutable to nativeExecutablePath, and if nativeExecutablePath is defined, I will call copyBinaryToStageDirectory instead copyJarsToStageDirectory

Correct?

I prefer to keep getArtifact as it is, and move binary related logic to copyBinaryToStageDirectory in package mojo.

Yes, I think nativeExecutablePath may be more flexible, and we no longer need to generate the path for executable

Hi @Flanker32 , I did not test all the changes yet.

Flanker32 · 2023-05-31T09:18:21Z

azure-functions-maven-plugin/src/main/java/com/microsoft/azure/maven/function/PackageMojo.java

@@ -348,10 +389,15 @@ protected void copyLocalSettingsJson() throws IOException {
        log.info(SAVING_LOCAL_SETTINGS_JSON);
        final File sourceLocalSettingsJsonFile = getLocalSettingsJsonFile();
        final File destLocalSettingsJsonFile = Paths.get(getDeploymentStagingDirectoryPath(), LOCAL_SETTINGS_JSON).toFile();
-        copyFilesWithDefaultContent(sourceLocalSettingsJsonFile, destLocalSettingsJsonFile, DEFAULT_LOCAL_SETTINGS_JSON);
+        String defaultContent = MessageFormat.format(DEFAULT_LOCAL_SETTINGS_JSON, getWorkerRuntime());


Do we need to overwrite users configuration for native artifact? As default content will not work if users have a local.settings.json in their workspace. Same for the host.json

I have one newbie question: local.settins.json is enough to test locally (without host.json)?
I think the plugin should not overwrite any json files created by the user.

Yes, you can test locally without the host.json file.
local.settings.json is used for local development and will not be published to Azure. [On the other hand, host.json is used for configuring the Functions host process when you run projects locally and in Azure, for log level for instance]

https://learn.microsoft.com/en-us/azure/azure-functions/functions-develop-local

@viniciusfcf @SandraAhlgrimm Thank you all for your help. Just want to confirm whether customHandler configuration is required in host.json? I just tried to remove following configuration and func seems broken.

If its required, do we need to add validation throw exception if user did not set it in host.json? Besides, I'm not sure whether we should provide default configuration as the following configuration should only works for quarkus, or is there any way we could detect whether the binary is a quarkus one or spring boot?

"customHandler": { "description": { "defaultExecutablePath": "quarkus-azure-functions-http-1.0.0-SNAPSHOT", "workingDirectory": "", "arguments": [ "-Dquarkus.http.port=${FUNCTIONS_CUSTOMHANDLER_PORT}" ] }, "enableForwardingHttpRequest": true }

Flanker32 · 2023-05-31T09:21:15Z

@viniciusfcf Thanks a lot for your efforts! Please see my comments in this PR.

Besides, I tried to run your demo project https://github.com/viniciusfcf/quarkus-azure-functions-http/tree/using-new-versions, but will get same error here https://github.com/viniciusfcf/quarkus-azure-functions-http/blob/using-new-versions/image.png, could you please help provide some guidance to resolve this issue? I tried both Mac M2 and Windows WSL (Ubuntu), both no luck.

Tried fixes in quarkusio/quarkus#33681, it works fine now

SandraAhlgrimm · 2023-06-20T09:31:53Z

Converting to Draft. I'll seek guidance from @Flanker32 , but in general I am in favor of this feature.

Can we convert back to Open PR please 👍

Flanker32 · 2023-06-20T12:10:00Z

...ions-maven-plugin/src/main/java/com/microsoft/azure/maven/function/AbstractFunctionMojo.java

@@ -239,6 +259,10 @@ protected File getArtifact() throws AzureToolkitRuntimeException {
        return result;
    }

+    protected boolean isNativeExecutable() {
+        return StringUtils.isEmpty(getNativeExecutablePath());


should be isNotEmpty?

Flanker32 · 2023-06-20T12:10:45Z

...ions-maven-plugin/src/main/java/com/microsoft/azure/maven/function/AbstractFunctionMojo.java

@@ -323,6 +347,7 @@ public Map<String, String> getTelemetryProperties() {
        result.put(DISABLE_APP_INSIGHTS_KEY, String.valueOf(isDisableAppInsights()));
        final boolean isDeployToFunctionSlot = getDeploymentSlotSetting() != null && StringUtils.isNotEmpty(getDeploymentSlotSetting().getName());
        result.put(FUNCTION_DEPLOY_TO_SLOT_KEY, String.valueOf(isDeployToFunctionSlot));
+        result.put(FUNCTION_NATIVE_EXECUTABLE_PATH, nativeExecutablePath);


please just track isNativeExecutable as path may include pii data

Flanker32 · 2023-06-20T12:50:28Z

@viniciusfcf @SandraAhlgrimm Thank you all for your help and efforts. Please see my comments in the PR.

Besides, for the configuration in host.json, just want to confirm whether customHandler configuration is required in host.json? If so, I think we should add validation in native binary cases and throw exception if use did not set it or the host.json is missing, as it seems that we could not provide a default configuration for this case (the default configuration in this PR seems only works for quarkus.

SandraAhlgrimm · 2023-07-13T07:47:15Z

@agoncal for review

Flanker32 · 2023-08-07T05:47:40Z

...e-lib/src/main/java/com/microsoft/azure/toolkit/lib/appservice/config/FunctionAppConfig.java

@@ -19,6 +19,8 @@ public class FunctionAppConfig extends AppServiceConfig {
    private boolean disableAppInsights;
    private String storageAccountName;
    private String storageAccountResourceGroup;
+    private String nativeExecutablePath;


Please remove these new parameters as FunctionAppConfig is used to create resource in Azure, which should not contains native binary configurations

wangmingliang-ms requested a review from Flanker32 May 30, 2023 01:34

viniciusfcf mentioned this pull request May 30, 2023

Use requestVirtualHttp only if not native quarkusio/quarkus#33681

Open

edburns marked this pull request as draft May 30, 2023 17:22

Flanker32 reviewed May 31, 2023

View reviewed changes

azure-functions-maven-plugin/src/main/java/com/microsoft/azure/maven/function/PackageMojo.java Outdated Show resolved Hide resolved

Flanker32 reviewed May 31, 2023

View reviewed changes

...e-lib/src/main/java/com/microsoft/azure/toolkit/lib/appservice/config/FunctionAppConfig.java Outdated Show resolved Hide resolved

Flanker32 reviewed May 31, 2023

View reviewed changes

viniciusfcf force-pushed the java-projects-native-deployments branch from 328dde5 to b5187b2 Compare May 31, 2023 15:26

viniciusfcf force-pushed the java-projects-native-deployments branch from b5187b2 to 98db9fe Compare June 12, 2023 02:33

viniciusfcf closed this Jun 12, 2023

viniciusfcf force-pushed the java-projects-native-deployments branch 2 times, most recently from 98db9fe to fd601b1 Compare June 12, 2023 02:38

viniciusfcf reopened this Jun 12, 2023

edburns previously approved these changes Jun 15, 2023

View reviewed changes

Flanker32 reviewed Jun 20, 2023

View reviewed changes

Flanker32 marked this pull request as ready for review June 20, 2023 12:16

viniciusfcf dismissed edburns’s stale review via d87d3b8 June 20, 2023 13:03

viniciusfcf force-pushed the java-projects-native-deployments branch from 7bc506a to d87d3b8 Compare June 20, 2023 13:03

edburns previously approved these changes Jul 14, 2023

View reviewed changes

edburns dismissed their stale review via 4489749 August 2, 2023 03:34

edburns force-pushed the java-projects-native-deployments branch from 891656f to 4489749 Compare August 2, 2023 03:34

viniciusfcf and others added 2 commits August 1, 2023 23:35

Generate native binary from java code

ba5ca8c

switch executeable path to isexecutable in telemetrics

3d95343

edburns force-pushed the java-projects-native-deployments branch from 4489749 to 3d95343 Compare August 2, 2023 03:35

Flanker32 reviewed Aug 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Generate native binary from java code #2302

WIP: Generate native binary from java code #2302

viniciusfcf commented May 29, 2023

viniciusfcf commented May 29, 2023

viniciusfcf commented May 29, 2023

edburns commented May 30, 2023

Flanker32 May 31, 2023

viniciusfcf May 31, 2023

Flanker32 Jun 1, 2023 •

edited

Loading

Flanker32 May 31, 2023 •

edited

Loading

viniciusfcf May 31, 2023 •

edited

Loading

Flanker32 Jun 1, 2023

viniciusfcf Jun 12, 2023

Flanker32 May 31, 2023 •

edited

Loading

viniciusfcf May 31, 2023

SandraAhlgrimm May 31, 2023

Flanker32 Jun 1, 2023 •

edited

Loading

Flanker32 commented May 31, 2023 •

edited

Loading

SandraAhlgrimm commented Jun 20, 2023

Flanker32 Jun 20, 2023

Flanker32 Jun 20, 2023

Flanker32 commented Jun 20, 2023

SandraAhlgrimm commented Jul 13, 2023

Flanker32 Aug 7, 2023

WIP: Generate native binary from java code #2302

Are you sure you want to change the base?

WIP: Generate native binary from java code #2302

Conversation

viniciusfcf commented May 29, 2023

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any relevant logs, screenshots, error output, etc.?

Any other comments?

Has this been tested?

viniciusfcf commented May 29, 2023

viniciusfcf commented May 29, 2023

edburns commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flanker32 Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Flanker32 May 31, 2023 • edited Loading

Choose a reason for hiding this comment

viniciusfcf May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flanker32 May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flanker32 Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Flanker32 commented May 31, 2023 • edited Loading

SandraAhlgrimm commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flanker32 commented Jun 20, 2023

SandraAhlgrimm commented Jul 13, 2023

Choose a reason for hiding this comment

Flanker32 Jun 1, 2023 •

edited

Loading

Flanker32 May 31, 2023 •

edited

Loading

viniciusfcf May 31, 2023 •

edited

Loading

Flanker32 May 31, 2023 •

edited

Loading

Flanker32 Jun 1, 2023 •

edited

Loading

Flanker32 commented May 31, 2023 •

edited

Loading