chore(dist): replace wget to curl to download swagger-ui #2277

VGalaxies · 2023-08-12T06:09:01Z

Purpose of the PR

related issue or PR:

Main Changes

replace wget by curl when downloading swagger-ui
silence the output of curl and tar commands
reuse the existing v4.15.5.tar.gz before downloading
avoid downloading swagger-ui in non-Linux platforms to prevent build failures (there might be a cross-platform build approach available 🤔)
wrapp the script content within <![CDATA[ ... ]]> blocks ensures that the script retains its original format when generating the dist.sh script (also suppresses automatic indentation)
remove intermediate files at the end of the script

An alternative approach, during the generation of the dist.sh script, only the ${final.name} property from the build process is utilized. It might be possible to separately store a dist.sh script within hugegraph-dist, then use sed during the build process to replace the value of ${final.name}, thereby avoiding the need to embed script content within the pom file.

Verifying these changes

Trivial rework / code cleanup without any test coverage. (No Need)
Already covered by existing tests, such as (please modify tests here).
Need tests and can be verified as follows:
- xxx

Does this PR potentially affect the following parts?

Documentation Status

Doc - TODO
Doc - Done
Doc - No Need

codecov · 2023-08-12T06:19:56Z

Codecov Report

Merging #2277 (f707fcb) into master (b02c2bd) will decrease coverage by 0.39%.
Report is 2 commits behind head on master.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #2277      +/-   ##
============================================
- Coverage     68.63%   68.24%   -0.39%     
- Complexity      977      981       +4     
============================================
  Files           498      498              
  Lines         40684    41240     +556     
  Branches       5681     5738      +57     
============================================
+ Hits          27922    28146     +224     
- Misses        10056    10363     +307     
- Partials       2706     2731      +25

see 73 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

hugegraph-dist/pom.xml

VGalaxies · 2023-08-14T14:02:07Z

also added maven-enforcer-plugin to check java and maven version for #2114 (comment)

hugegraph-dist/pom.xml

VGalaxies · 2023-08-22T09:52:51Z

The order of executions related to packaging should be assembly-hugegraph -> install-swagger-ui (copy and remove) -> tar-package. The download-swagger-ui goal only needs to be completed before install-swagger-ui (currently bound to the prepare-package phase).

To control the execution order of different goals within the same phase, consider declaring the above executions in sequence within the pluginManagement of the root pom. Also use conditions like os and file to ensure that these goals are executed only within the Unix or macOS environment, specifically within the hugegraph-dist module.

pom.xml

imbajin · 2023-08-23T02:46:14Z

pom.xml

+            </build>
+        </profile>
+        <profile>
+            <id>assembly-hugegraph</id>


after we move the profile, what's the proper building command?

Before:
mvn clean install -Papache-release -DskipTests for building apache-release source/binary package

Now: ?

When a profile is activated through the implicit profile activation mechanism, the explicit profile activation mechanism within the same pom becomes ineffective.

So it's necessary to explicitly specify the profiles:

mvn clean install -Papache-release,assembly-hugegraph,tar-package -DskipTests

ref: https://maven.apache.org/guides/introduction/introduction-to-profiles.html#explicit-profile-activation

When a profile is activated through the implicit profile activation mechanism, the explicit profile activation mechanism within the same pom becomes ineffective.

So it's necessary to explicitly specify the profiles:

mvn clean install -Papache-release,assembly-hugegraph,tar-package -DskipTests

ref: maven.apache.org/guides/introduction/introduction-to-profiles.html#explicit-profile-activation

The relevant scripts and documentation need to be updated later.

imbajin · 2023-08-23T02:48:56Z

pom.xml

+                            <id>download-swagger-ui</id>
+                            <phase>prepare-package</phase>


seems this step will not be inherited by other modules?

VGalaxies · 2023-08-23T05:01:34Z

hugegraph-dist/pom.xml

+        <profile>
+            <id>tar-package</id>
+            <build>
+                <plugins>
+                    <plugin>
+                        <groupId>org.apache.maven.plugins</groupId>
+                        <artifactId>maven-antrun-plugin</artifactId>
+                        <version>1.8</version>
+                        <executions>
+                            <execution>
+                                <id>tar-package</id>
+                                <phase>package</phase>
+                                <goals>
+                                    <goal>run</goal>
+                                </goals>
+                                <configuration>
+                                    <target>
+                                        <tar destfile="${final.destfile}" compression="gzip">
+                                            <tarfileset dir="${top.level.dir}/" filemode="755">
+                                                <include name="${final.name}/**"/>
+                                            </tarfileset>
+                                        </tar>
+                                    </target>
+                                </configuration>
+                            </execution>
+                        </executions>
+                    </plugin>
+                </plugins>
+            </build>
+            <activation>
+                <activeByDefault>true</activeByDefault>
+            </activation>
+        </profile>


After introducing pd and store (#2265), the relevant package goals could be considered for reuse.

After introducing pd and store (#2265), the relevant package goals could be considered for reuse.

@msgui could also take a look for this PR, and it will be helpful when u refactor the new struct

imbajin · 2023-08-23T07:08:27Z

hugegraph-dist/pom.xml

+        <profile>
+            <id>tar-package</id>
+            <build>
+                <plugins>
+                    <plugin>
+                        <groupId>org.apache.maven.plugins</groupId>
+                        <artifactId>maven-antrun-plugin</artifactId>
+                        <version>1.8</version>
+                        <executions>
+                            <execution>
+                                <id>tar-package</id>
+                                <phase>package</phase>
+                                <goals>
+                                    <goal>run</goal>
+                                </goals>
+                                <configuration>
+                                    <target>
+                                        <tar destfile="${final.destfile}" compression="gzip">
+                                            <tarfileset dir="${top.level.dir}/" filemode="755">
+                                                <include name="${final.name}/**"/>
+                                            </tarfileset>
+                                        </tar>
+                                    </target>
+                                </configuration>
+                            </execution>
+                        </executions>
+                    </plugin>
+                </plugins>
+            </build>
+            <activation>
+                <activeByDefault>true</activeByDefault>
+            </activation>
+        </profile>


After introducing pd and store (#2265), the relevant package goals could be considered for reuse.

@msgui could also take a look for this PR, and it will be helpful when u refactor the new struct

imbajin · 2023-08-23T07:09:33Z

hugegraph-dist/pom.xml

        </plugins>
+
+        <pluginManagement>


need pluginManagement?

BTW, all the CI failed now 🔎

need pluginManagement?

BTW, all the CI failed now 🔎

for reuse the configuration of the maven-antrun-plugin

because of chore(dist): replace wget to curl to download swagger-ui #2277 (comment), fix in 880d283 (#2277)

This reverts commit 880d283.

VGalaxies · 2023-08-23T13:14:15Z

Following this discussion:

Through researching relevant information, we found that the semantics of activeByDefault are somewhat counterintuitive. In a particular pom, if a profile is activated through:

Specifying it via the command line using -Pxxx, or
Defining it in the Maven settings using <activeProfiles>, or
Implicit profile activation, such as jdk, os, property, file

Then, within the same pom, a profile configured with activeByDefault set to true will NOT be activated.

To avoid explicitly specifying profiles suppressed from activation due to the aforementioned mechanisms via -Pxxx in the command line, we referred to the approach outlined in the official documentation:

<profiles>
  <profile>
    <id>xxx</id>
    <activation>
      <property>
        <name>!skip-xxx</name>
      </property>
    </activation>
    ...
  </profile>
</profiles>

Using the property mechanism within implicit profile activation to activate profiles configured with activeByDefault set to true.

Additionally, it's important to minimize occurrences of identical property names across different profiles. For example:

In a scenario where both profiles are activated, the value of env is test (dev is loaded and then overridden). Explicitly specifying -Pdev could result in the value of env becoming dev, which might introduce potential issues.

References:

VGalaxies · 2023-08-24T10:25:50Z

TODO: We have observed that when the core-test profile under the root pom is activated using the aforementioned approach, it leads to failures while running the majority of backend api-test cases. For more details, please refer to: GitHub Actions Run.

also provide the expected result: GitHub Actions Run

There might be an issue with the run-core-test.sh script.

imbajin

LGTM

Main Changes: 1. replace `wget` by `curl` when downloading `swagger-ui` 2. silence the output of `curl` and `tar` commands 3. reuse the existing `v4.15.5.tar.gz` before downloading 4. avoid downloading `swagger-ui` in non-Linux platforms to prevent build failures (there might be a cross-platform build approach available 🤔) 5. wrapp the script content within `<![CDATA[ ... ]]>` blocks ensures that the script retains its original format when generating the `dist.sh` script (also suppresses automatic indentation) 6. remove intermediate files at the end of the script **An alternative approach**, during the generation of the `dist.sh` script, only the `${final.name}` property from the build process is utilized. It might be possible to separately store a `dist.sh` script within hugegraph-dist, then use `sed` during the build process to replace the value of `${final.name}`, **thereby avoiding the need to embed script content within the pom file**. --------- Co-authored-by: imbajin <[email protected]>

build: optimize 'download-swagger-ui' in hugegraph-dist

3ae4aac

imbajin reviewed Aug 12, 2023

View reviewed changes

hugegraph-dist/pom.xml Outdated Show resolved Hide resolved

imbajin mentioned this pull request May 15, 2023

[Summary] Server Release v1.0 TODOs #2011

Closed

15 tasks

imbajin self-assigned this Aug 14, 2023

imbajin added this to the 1.5.0 milestone Aug 14, 2023

imbajin changed the title ~~build(dist): improve download-swagger-ui in hugegraph-dist~~ chore(dist): improve download-swagger-ui in hugegraph-dist Aug 14, 2023

imbajin changed the title ~~chore(dist): improve download-swagger-ui in hugegraph-dist~~ chore(dist): replace wget to curl to download swagger-ui Aug 14, 2023

chore: skip the swagger building if network/curl broken

0a9215f

VGalaxies requested a review from imbajin August 14, 2023 08:49

imbajin mentioned this pull request Aug 14, 2023

[Summary] Support Swagger-ui & enhance it #2088

Open

5 tasks

imbajin reviewed Aug 14, 2023

View reviewed changes

hugegraph-dist/pom.xml Show resolved Hide resolved

imbajin requested review from z7658329 and simon824 August 14, 2023 10:21

VGalaxies added 2 commits August 14, 2023 21:39

chore: extract embedded script as a separate file

0822ac4

chore: add maven-enforcer-plugin to check java and maven version

eebcded

VGalaxies requested a review from imbajin August 14, 2023 14:01

VGalaxies and others added 2 commits August 14, 2023 22:10

chore: add LICENSE for dist.sh

97e9269

Update dist.sh

2782366

imbajin reviewed Aug 16, 2023

View reviewed changes

hugegraph-dist/pom.xml Outdated Show resolved Hide resolved

chore: move download-swagger-ui to root pom plugin management

1534605

VGalaxies requested a review from imbajin August 21, 2023 07:35

imbajin previously approved these changes Aug 21, 2023

View reviewed changes

z7658329 reviewed Aug 22, 2023

View reviewed changes

hugegraph-dist/pom.xml Show resolved Hide resolved

chore: move profiles in hugegraph-dist to root pom

509a4e2

VGalaxies dismissed imbajin’s stale review via 509a4e2 August 22, 2023 09:39

VGalaxies commented Aug 22, 2023

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

VGalaxies requested a review from z7658329 August 22, 2023 09:54

VGalaxies requested a review from imbajin August 22, 2023 09:54

VGalaxies added 3 commits August 22, 2023 17:58

chore: assemble for non-unix os

645258c

chore: rename profile id

df0779c

chore: tar package for non-unix os

db9a9e3

imbajin reviewed Aug 23, 2023

View reviewed changes

VGalaxies added 2 commits August 23, 2023 12:28

chore: move package goals to dist pom

264c41c

chore: comment dependency convergence

ff7a779

VGalaxies commented Aug 23, 2023

View reviewed changes

imbajin reviewed Aug 23, 2023

View reviewed changes

VGalaxies added 4 commits August 23, 2023 15:54

chore: fix mvn package args

880d283

revert "chore: fix mvn package args"

66d0241

This reverts commit 880d283.

chore: activate profile by property

e5929e2

chore: execute enforcer only in root pom

e5f3642

VGalaxies requested a review from imbajin August 23, 2023 13:16

chore: revert core-test activation

7d9e991

VGalaxies and others added 2 commits August 24, 2023 18:51

doc: add comment for property activation

3b480f6

Update pom.xml

f707fcb

imbajin approved these changes Aug 25, 2023

View reviewed changes

z7658329 approved these changes Aug 25, 2023

View reviewed changes

imbajin merged commit 77c7612 into apache:master Aug 25, 2023

imbajin mentioned this pull request Aug 25, 2023

[Bug] HG hubble build fail apache/incubator-hugegraph-toolchain#513

Closed

1 task

VGalaxies deleted the dist-build branch August 29, 2023 12:59

imbajin mentioned this pull request Sep 9, 2023

feat: support Cassandra with docker-compose in server #2307

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(dist): replace wget to curl to download swagger-ui #2277

chore(dist): replace wget to curl to download swagger-ui #2277

VGalaxies commented Aug 12, 2023 •

edited by imbajin

Loading

codecov bot commented Aug 12, 2023 •

edited

Loading

VGalaxies commented Aug 14, 2023

VGalaxies commented Aug 22, 2023 •

edited

Loading

imbajin Aug 23, 2023

VGalaxies Aug 23, 2023

VGalaxies Aug 23, 2023

imbajin Aug 23, 2023

VGalaxies Aug 23, 2023

imbajin Aug 23, 2023

imbajin Aug 23, 2023

imbajin Aug 23, 2023 •

edited

Loading

VGalaxies Aug 23, 2023

VGalaxies commented Aug 23, 2023 •

edited

Loading

VGalaxies commented Aug 24, 2023 •

edited

Loading

imbajin left a comment

chore(dist): replace wget to curl to download swagger-ui #2277

chore(dist): replace wget to curl to download swagger-ui #2277

Conversation

VGalaxies commented Aug 12, 2023 • edited by imbajin Loading

Purpose of the PR

Main Changes

Verifying these changes

Does this PR potentially affect the following parts?

Documentation Status

codecov bot commented Aug 12, 2023 • edited Loading

Codecov Report

VGalaxies commented Aug 14, 2023

VGalaxies commented Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imbajin Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VGalaxies commented Aug 23, 2023 • edited Loading

VGalaxies commented Aug 24, 2023 • edited Loading

imbajin left a comment

Choose a reason for hiding this comment

VGalaxies commented Aug 12, 2023 •

edited by imbajin

Loading

codecov bot commented Aug 12, 2023 •

edited

Loading

VGalaxies commented Aug 22, 2023 •

edited

Loading

imbajin Aug 23, 2023 •

edited

Loading

VGalaxies commented Aug 23, 2023 •

edited

Loading

VGalaxies commented Aug 24, 2023 •

edited

Loading