You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "VGalaxies (via GitHub)" <gi...@apache.org> on 2023/08/12 06:09:03 UTC
[GitHub] [incubator-hugegraph] VGalaxies opened a new pull request, #2277: build(dist): improve download-swagger-ui in hugegraph-dist
VGalaxies opened a new pull request, #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277
<!--
Thank you very much for contributing to Apache HugeGraph, we are happy that you want to help us improve it!
Here are some tips for you:
1. If this is your first time, please read the [contributing guidelines](https://github.com/apache/hugegraph/blob/master/CONTRIBUTING.md)
2. If a PR fix/close an issue, type the message "close xxx" (xxx is the link of related
issue) in the content, GitHub will auto link it (Required)
3. Name the PR title in "Google Commit Format", start with "feat | fix | perf | refactor | doc | chore",
such like: "feat(core): support the PageRank algorithm" or "fix: wrong break in the compute loop" (module is optional)
skip it if you are unsure about which is the best component.
4. One PR address one issue, better not to mix up multiple issues.
5. Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]` (or click it directly after
published)
-->
## Purpose of the PR
close https://github.com/apache/incubator-hugegraph/issues/2223
related issue or PR:
- https://github.com/apache/incubator-hugegraph/issues/2011
- https://github.com/apache/incubator-hugegraph/pull/2262#discussion_r1291153745
<!--
Please explain more context in this section, clarify why the changes are needed.
e.g:
- If you propose a new API, clarify the use case for a new API.
- If you fix a bug, you can clarify why it is a bug, and should be associated with an issue.
-->
## Main Changes
<!-- Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue. These change logs are helpful for better ant faster reviews.)
For example:
- If you introduce a new feature, please show detailed design here or add the link of design documentation.
- If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
- If there is a discussion in the mailing list, please add the link. -->
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
6. remove intermediate files at the end of the script
## Verifying these changes
<!-- Please pick the proper options below -->
- [x] 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?
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] Nope
- [ ] Dependencies (add/update license info) <!-- Don't forget to add/update the info in "LICENSE" & "NOTICE" files (both in root & dist module) -->
- [x] Modify configurations
- [ ] The public API
- [ ] Other affects (typed here)
## Documentation Status
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `Doc - TODO` <!-- Your PR changes impact docs and you will update later -->
- [ ] `Doc - Done` <!-- Related docs have been already added or updated -->
- [x] `Doc - No Need` <!-- Your PR changes don't impact/need docs -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1302577170
##########
hugegraph-dist/pom.xml:
##########
@@ -168,51 +144,162 @@
</filesets>
</configuration>
</plugin>
-
- <plugin>
- <artifactId>maven-antrun-plugin</artifactId>
- <executions>
- <execution>
- <id>download-swagger-ui</id>
- <phase>package</phase>
- <goals>
- <goal>run</goal>
- </goals>
- <configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
- </configuration>
- </execution>
- <execution>
- <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>
+
+ <pluginManagement>
Review Comment:
need `pluginManagement`?
##########
hugegraph-dist/pom.xml:
##########
@@ -168,51 +144,162 @@
</filesets>
</configuration>
</plugin>
-
- <plugin>
- <artifactId>maven-antrun-plugin</artifactId>
- <executions>
- <execution>
- <id>download-swagger-ui</id>
- <phase>package</phase>
- <goals>
- <goal>run</goal>
- </goals>
- <configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
- </configuration>
- </execution>
- <execution>
- <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>
+
+ <pluginManagement>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-assembly-plugin</artifactId>
+ <version>2.4</version>
+ <executions>
+ <execution>
+ <id>assembly-hugegraph</id>
+ <phase>package</phase>
+ <goals>
+ <goal>single</goal>
+ </goals>
+ <configuration>
+ <attach>false</attach>
+ <appendAssemblyId>false</appendAssemblyId>
+ <outputDirectory>${top.level.dir}
+ </outputDirectory>
+ <descriptor>
+ ${assembly.descriptor.dir}/assembly.xml
+ </descriptor>
+ <finalName>${final.name}</finalName>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-antrun-plugin</artifactId>
+ <version>1.8</version>
+ <executions>
+ <execution>
+ <id>download-swagger-ui</id>
+ <phase>prepare-package</phase>
+ <goals>
+ <goal>run</goal>
+ </goals>
+ <configuration>
+ <target>
+ <exec executable="${shell-executable}"
+ dir="${project.basedir}"
+ failonerror="false">
+ <arg line="./dist.sh"/>
+ </exec>
+ </target>
+ </configuration>
+ </execution>
+ <execution>
+ <id>install-swagger-ui</id>
+ <phase>package</phase>
+ <goals>
+ <goal>run</goal>
+ </goals>
+ <configuration>
+ <target>
+ <exec executable="cp"
+ dir="${project.basedir}"
+ failonerror="false">
+ <arg value="-r"/>
+ <arg value="swagger-ui/dist"/>
+ <arg value="../${final.name}/swagger-ui"/>
+ </exec>
+ <exec executable="rm"
+ dir="${project.basedir}"
+ failonerror="false">
+ <arg value="-rf"/>
+ <arg value="swagger-ui"/>
+ </exec>
+ </target>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </pluginManagement>
</build>
+
+ <profiles>
+ <profile>
+ <id>assembly-hugegraph</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-assembly-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
+ <activation>
+ <activeByDefault>true</activeByDefault>
+ </activation>
+ </profile>
+ <profile>
+ <id>unix-package</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-antrun-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
+ <activation>
+ <os>
+ <family>unix</family>
+ <name>Linux</name>
+ </os>
+ </activation>
+ </profile>
+ <profile>
+ <id>mac-package</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-antrun-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
+ <activation>
+ <os>
+ <family>mac</family>
+ </os>
+ </activation>
+ </profile>
+ <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>
Review Comment:
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1296255176
##########
hugegraph-dist/pom.xml:
##########
@@ -212,6 +229,19 @@
</configuration>
</execution>
</executions>
+ <dependencies>
+ <dependency>
+ <groupId>ant-contrib</groupId>
+ <artifactId>ant-contrib</artifactId>
+ <version>1.0b3</version>
+ <exclusions>
+ <exclusion>
+ <groupId>ant</groupId>
+ <artifactId>ant</artifactId>
+ </exclusion>
+ </exclusions>
Review Comment:
we still need it for `if/else` tag?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1297541243
##########
hugegraph-dist/pom.xml:
##########
@@ -212,6 +229,19 @@
</configuration>
</execution>
</executions>
+ <dependencies>
+ <dependency>
+ <groupId>ant-contrib</groupId>
+ <artifactId>ant-contrib</artifactId>
+ <version>1.0b3</version>
+ <exclusions>
+ <exclusion>
+ <groupId>ant</groupId>
+ <artifactId>ant</artifactId>
+ </exclusion>
+ </exclusions>
Review Comment:
no need，fine to skip download in windows
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: build(dist): improve download-swagger-ui in hugegraph-dist
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1292384879
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
maybe we could also check if curl exist in os env？ skip the swagger building if network/curl broken（do we need it？）
In general，the building shouldn't be bounded with outside network？
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
maybe we could also check if curl exist in os env？ skip the swagger building if network/curl broken（do we need it？）
In general，the building shouldn't be bounded with outside network？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1301367703
##########
hugegraph-dist/pom.xml:
##########
@@ -215,4 +193,40 @@
</plugin>
</plugins>
</build>
+
+ <profiles>
+ <profile>
+ <id>unix-download-swagger-ui</id>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-antrun-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
Review Comment:
@z7658329 done in [`509a4e2` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/509a4e27f8f58807be4c36019b7eb1330b01d63a)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#issuecomment-1687859941
The order of executions related to packaging should be `assembly-hugegraph` -> `install-swagger-ui` (copy and remove) -> `tar-package`. The `download-swagger-ui` task 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 based on `os` and `file` to ensure that these goals are executed only within the Unix or macOS environment, specifically within the `hugegraph-dist` module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1297542573
##########
hugegraph-dist/pom.xml:
##########
@@ -212,6 +229,19 @@
</configuration>
</execution>
</executions>
+ <dependencies>
+ <dependency>
+ <groupId>ant-contrib</groupId>
+ <artifactId>ant-contrib</artifactId>
+ <version>1.0b3</version>
+ <exclusions>
+ <exclusion>
+ <groupId>ant</groupId>
+ <artifactId>ant</artifactId>
+ </exclusion>
+ </exclusions>
Review Comment:
> To avoid downloading swagger-ui on non-Linux platforms, the utilization of the `if` tag from `ant-contrib` is demonstrated here.
>
> An alternative [approach](https://maven.apache.org/guides/introduction/introduction-to-profiles.html#OS) could involve controlling profile activation through the `os` attribute. **However, it seems tricky to activate the same profile for both Mac and Linux simultaneously.**
>
> **In order to reuse the configuration of the `maven-antrun-plugin`**, I'm considering setting up the `download-swagger-ui` configuration in the root pom's `pluginManagement`. In This way, it can be reused in `hugegraph-dist`, see [`1534605` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/1534605ae8ab64a2d601fd3930ec19967bf8cd27).
>
nice refactor （try it）
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: build(dist): improve download-swagger-ui in hugegraph-dist
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1292717963
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
- I mean check but not exit directly if miss it（skip instead）
- like many machine could use inner “self-host repository”（like gitlab VS github） to download deps，and next time they will reuse it from local .m2 repo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: build(dist): improve download-swagger-ui in hugegraph-dist
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1292717963
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
- I mean check but not exit directly if miss it（skip download swagger instead）
- like many machines could use inner “self-host repository”（like gitlab VS github） to download deps，and next time they will reuse it from local .m2 repo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: build(dist): improve download-swagger-ui in hugegraph-dist
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1292976004
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
> Does the following result meet your expectations?
>
> 
>
> Similar outcomes can be expected in the case of network abnormalities.
LGTM, the warning message could be "ERROR: Please install `curl` first if you need `swagger-ui`"
BTW, could also skip download if network bad like `curl xxx || exit`?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#issuecomment-1677374151
also added maven-enforcer-plugin to check java and maven version for https://github.com/apache/incubator-hugegraph/issues/2114#issuecomment-1453091210
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1302507017
##########
pom.xml:
##########
@@ -676,8 +747,118 @@
</gpgArguments>
</configuration>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>enforce-all</id>
+ <goals>
+ <goal>enforce</goal>
+ </goals>
+ <configuration>
+ <rules>
+ <DependencyConvergence/>
+ <requireJavaVersion>
+ <version>[1.8,12)</version>
+ </requireJavaVersion>
+ <requireMavenVersion>
+ <version>[3.5.0,)</version>
+ </requireMavenVersion>
+ </rules>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+ </profile>
+ <profile>
+ <id>assembly-hugegraph</id>
Review Comment:
> 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:
>
> ```shell
> mvn clean install -Papache-release,assembly-hugegraph,tar-package -DskipTests
> ```
>
> > ref: [maven.apache.org/guides/introduction/introduction-to-profiles.html#explicit-profile-activation](https://maven.apache.org/guides/introduction/introduction-to-profiles.html#explicit-profile-activation)
The relevant scripts and documentation need to be updated later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1293500552
##########
hugegraph-dist/pom.xml:
##########
Review Comment:
@imbajin done in [`0822ac4` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/0822ac40b680f570c72408184f49fd742444b43b)
To avoid explicitly modifying the script using `ReplaceRegExp`, a separate script file is used solely for downloading. The necessary copy and remove tasks are accomplished in the pom file using `exec`.
By setting `failonerror` to false, the swagger-ui downloading is skipped if the network/curl is broken.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1293280897
##########
hugegraph-dist/pom.xml:
##########
Review Comment:
<img width="1104" alt="image" src="https://github.com/apache/incubator-hugegraph/assets/17706099/7b32a1fb-ad91-477f-add8-0e637c9dc3a3">
Agree to extract as a **separate file** as suggested by you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: build(dist): improve download-swagger-ui in hugegraph-dist
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1292717963
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
- I mean check but not exit directly if miss it（skip instead）
- like many machines could use inner “self-host repository”（like gitlab VS github） to download deps，and next time they will reuse it from local .m2 repo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] z7658329 commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "z7658329 (via GitHub)" <gi...@apache.org>.
z7658329 commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1301157152
##########
hugegraph-dist/pom.xml:
##########
@@ -215,4 +193,40 @@
</plugin>
</plugins>
</build>
+
+ <profiles>
+ <profile>
+ <id>unix-download-swagger-ui</id>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-antrun-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
Review Comment:
can extract to root pom file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1296625306
##########
hugegraph-dist/pom.xml:
##########
@@ -212,6 +229,19 @@
</configuration>
</execution>
</executions>
+ <dependencies>
+ <dependency>
+ <groupId>ant-contrib</groupId>
+ <artifactId>ant-contrib</artifactId>
+ <version>1.0b3</version>
+ <exclusions>
+ <exclusion>
+ <groupId>ant</groupId>
+ <artifactId>ant</artifactId>
+ </exclusion>
+ </exclusions>
Review Comment:
To avoid downloading swagger-ui on non-Linux platforms, the utilization of the `if` tag from `ant-contrib` is demonstrated here.
An alternative [approach](https://maven.apache.org/guides/introduction/introduction-to-profiles.html#OS) could involve controlling profile activation through the `os` attribute. **However, it seems tricky to activate the same profile for both Mac and Linux simultaneously.**
**In order to reuse the configuration of the `maven-antrun-plugin`**, I'm considering setting up the `download-swagger-ui` configuration in the root pom's `pluginManagement`. In this way, it can be reused for `hugegraph-dist`, see [`1534605` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/1534605ae8ab64a2d601fd3930ec19967bf8cd27).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: build(dist): improve download-swagger-ui in hugegraph-dist
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1292928956
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
@imbajin Using this snippet without having curl installed:
```bash
curl --version >/dev/null 2>&1 || (echo 'WARN: Please install `curl` first' && exit)
```
Does the following result meet your expectations?
Similar outcomes can be expected in the case of network abnormalities.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1296255176
##########
hugegraph-dist/pom.xml:
##########
@@ -212,6 +229,19 @@
</configuration>
</execution>
</executions>
+ <dependencies>
+ <dependency>
+ <groupId>ant-contrib</groupId>
+ <artifactId>ant-contrib</artifactId>
+ <version>1.0b3</version>
+ <exclusions>
+ <exclusion>
+ <groupId>ant</groupId>
+ <artifactId>ant</artifactId>
+ </exclusion>
+ </exclusions>
Review Comment:
we still need it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: build(dist): improve download-swagger-ui in hugegraph-dist
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1292717963
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
- I mean check but not exit if miss it（skip instead）
- like many machine could use inner “self-host repository”（like gitlab VS github） to download deps，and next time they will reuse it from local .m2 repo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#issuecomment-1689943936
Following this [discussion](https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1302507017):
Through researching relevant information, we found that the semantics of `activeByDefault` are somewhat counterintuitive. In a particular pom, if a profile is activated through:
1. Specifying it via the command line using `-Pxxx`
2. Defining it in the Maven settings using `<activeProfiles>`
3. 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:
```xml
<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:
1. [Maven Introduction to Profiles - Property Activation](https://maven.apache.org/guides/introduction/introduction-to-profiles.html#Property)
2. [Stack Overflow - How to Keep Maven Profiles which are activeByDefault active](https://stackoverflow.com/questions/5309379/how-to-keep-maven-profiles-which-are-activebydefault-active-even-if-another-prof)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1302570590
##########
hugegraph-dist/pom.xml:
##########
@@ -215,4 +193,40 @@
</plugin>
</plugins>
</build>
+
+ <profiles>
+ <profile>
+ <id>unix-download-swagger-ui</id>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-antrun-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
Review Comment:
> can extract to root pom file?
better to keep them in dist module now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1302381882
##########
pom.xml:
##########
@@ -676,8 +747,118 @@
</gpgArguments>
</configuration>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>enforce-all</id>
+ <goals>
+ <goal>enforce</goal>
+ </goals>
+ <configuration>
+ <rules>
+ <DependencyConvergence/>
+ <requireJavaVersion>
+ <version>[1.8,12)</version>
+ </requireJavaVersion>
+ <requireMavenVersion>
+ <version>[3.5.0,)</version>
+ </requireMavenVersion>
+ </rules>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+ </profile>
+ <profile>
+ <id>assembly-hugegraph</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-assembly-plugin</artifactId>
+ </plugin>
</plugins>
</build>
+ <activation>
+ <file>
+ <exists>dist.sh</exists>
+ </file>
+ </activation>
+ </profile>
+ <profile>
+ <id>unix-package</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-antrun-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
+ <activation>
+ <os>
+ <family>unix</family>
+ <name>Linux</name>
Review Comment:
`Unix` VS `MacOS` seems not right? (better to use `Linux` directly)
##########
pom.xml:
##########
@@ -676,8 +747,118 @@
</gpgArguments>
</configuration>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>enforce-all</id>
+ <goals>
+ <goal>enforce</goal>
+ </goals>
+ <configuration>
+ <rules>
+ <DependencyConvergence/>
+ <requireJavaVersion>
+ <version>[1.8,12)</version>
+ </requireJavaVersion>
+ <requireMavenVersion>
+ <version>[3.5.0,)</version>
+ </requireMavenVersion>
+ </rules>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+ </profile>
+ <profile>
+ <id>assembly-hugegraph</id>
Review Comment:
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: ?
##########
pom.xml:
##########
@@ -453,6 +453,77 @@
<consoleOutput>true</consoleOutput>
</configuration>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-assembly-plugin</artifactId>
+ <version>2.4</version>
+ <executions>
+ <execution>
+ <id>assembly-hugegraph</id>
+ <phase>package</phase>
+ <goals>
+ <goal>single</goal>
+ </goals>
+ <configuration>
+ <attach>false</attach>
+ <appendAssemblyId>false</appendAssemblyId>
+ <outputDirectory>${top.level.dir}
+ </outputDirectory>
+ <descriptor>
+ ${assembly.descriptor.dir}/assembly.xml
+ </descriptor>
+ <finalName>${final.name}</finalName>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-antrun-plugin</artifactId>
+ <version>1.8</version>
+ <executions>
+ <execution>
+ <id>download-swagger-ui</id>
+ <phase>prepare-package</phase>
Review Comment:
seems this step will not be inherited by other modules?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1302381882
##########
pom.xml:
##########
@@ -676,8 +747,118 @@
</gpgArguments>
</configuration>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>enforce-all</id>
+ <goals>
+ <goal>enforce</goal>
+ </goals>
+ <configuration>
+ <rules>
+ <DependencyConvergence/>
+ <requireJavaVersion>
+ <version>[1.8,12)</version>
+ </requireJavaVersion>
+ <requireMavenVersion>
+ <version>[3.5.0,)</version>
+ </requireMavenVersion>
+ </rules>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+ </profile>
+ <profile>
+ <id>assembly-hugegraph</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-assembly-plugin</artifactId>
+ </plugin>
</plugins>
</build>
+ <activation>
+ <file>
+ <exists>dist.sh</exists>
+ </file>
+ </activation>
+ </profile>
+ <profile>
+ <id>unix-package</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-antrun-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
+ <activation>
+ <os>
+ <family>unix</family>
+ <name>Linux</name>
Review Comment:
`Unix` VS `MacOS` seems not right? (better to use `Linux` directly if we compare to `MacOS`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: build(dist): improve download-swagger-ui in hugegraph-dist
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1292976004
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
> Does the following result meet your expectations?
>
> 
>
> Similar outcomes can be expected in the case of network abnormalities.
LGTM, the warb message could be "ERROR: Please install `curl` first if you need `swagger-ui`"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: build(dist): improve download-swagger-ui in hugegraph-dist
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1292976004
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
> Does the following result meet your expectations?
>
> 
>
> Similar outcomes can be expected in the case of network abnormalities.
LGTM, the warning message could be "ERROR: Please install `curl` first if you need `swagger-ui`"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1292928956
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
@imbajin Using this snippet without having curl installed:
```bash
curl --version >/dev/null 2>&1 || echo 'WARN: Please install `curl` first' && exit
```
Does the following result meet your expectations?
Similar outcomes can be expected in the case of network abnormalities.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] codecov[bot] commented on pull request #2277: build(dist): improve download-swagger-ui in hugegraph-dist
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#issuecomment-1675734964
## [Codecov](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2277?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#2277](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2277?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (3ae4aac) into [master](https://app.codecov.io/gh/apache/incubator-hugegraph/commit/b02c2bdaa7c88ee0dd3dd89f8e8beb386c24ed17?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (b02c2bd) will **decrease** coverage by `19.97%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #2277 +/- ##
=============================================
- Coverage 68.63% 48.66% -19.97%
+ Complexity 977 652 -325
=============================================
Files 498 498
Lines 40684 40681 -3
Branches 5681 5668 -13
=============================================
- Hits 27922 19798 -8124
- Misses 10056 18749 +8693
+ Partials 2706 2134 -572
```
[see 202 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2277/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1293177742
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
> > Does the following result meet your expectations?
> > 
> > Similar outcomes can be expected in the case of network abnormalities.
>
> LGTM, the warning message could be "ERROR: Please install `curl` first if you need `swagger-ui`"
>
> BTW, could also skip download if network bad like `curl xxx || exit`?)
Sure, the corresponding modifications can be found in [`0a9215f` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/0a9215f5ccc83dcacaf80142c0910a58039323be)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1302502303
##########
pom.xml:
##########
@@ -676,8 +747,118 @@
</gpgArguments>
</configuration>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>enforce-all</id>
+ <goals>
+ <goal>enforce</goal>
+ </goals>
+ <configuration>
+ <rules>
+ <DependencyConvergence/>
+ <requireJavaVersion>
+ <version>[1.8,12)</version>
+ </requireJavaVersion>
+ <requireMavenVersion>
+ <version>[3.5.0,)</version>
+ </requireMavenVersion>
+ </rules>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+ </profile>
+ <profile>
+ <id>assembly-hugegraph</id>
Review Comment:
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:
```bash
mvn clean install -Papache-release,assembly-hugegraph,tar-package -DskipTests
```
> ref: https://maven.apache.org/guides/introduction/introduction-to-profiles.html#explicit-profile-activation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1296625306
##########
hugegraph-dist/pom.xml:
##########
@@ -212,6 +229,19 @@
</configuration>
</execution>
</executions>
+ <dependencies>
+ <dependency>
+ <groupId>ant-contrib</groupId>
+ <artifactId>ant-contrib</artifactId>
+ <version>1.0b3</version>
+ <exclusions>
+ <exclusion>
+ <groupId>ant</groupId>
+ <artifactId>ant</artifactId>
+ </exclusion>
+ </exclusions>
Review Comment:
To avoid downloading swagger-ui on non-Linux platforms, the utilization of the `if` tag from `ant-contrib` is demonstrated here.
An alternative [approach](https://maven.apache.org/guides/introduction/introduction-to-profiles.html#OS) could involve controlling profile activation through the `os` attribute. **However, it seems tricky to activate the same profile for both Mac and Linux simultaneously.**
> ref: https://issues.apache.org/jira/browse/MNG-3328
**In order to reuse the configuration of the `maven-antrun-plugin`**, I'm considering setting up the `download-swagger-ui` configuration in the root pom's `pluginManagement`. In this way, it can be reused for `hugegraph-dist`, see [`1534605` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/1534605ae8ab64a2d601fd3930ec19967bf8cd27).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1296625306
##########
hugegraph-dist/pom.xml:
##########
@@ -212,6 +229,19 @@
</configuration>
</execution>
</executions>
+ <dependencies>
+ <dependency>
+ <groupId>ant-contrib</groupId>
+ <artifactId>ant-contrib</artifactId>
+ <version>1.0b3</version>
+ <exclusions>
+ <exclusion>
+ <groupId>ant</groupId>
+ <artifactId>ant</artifactId>
+ </exclusion>
+ </exclusions>
Review Comment:
To avoid downloading swagger-ui on non-Linux platforms, the utilization of the `if` tag from `ant-contrib` is demonstrated here.
An alternative [approach](https://maven.apache.org/guides/introduction/introduction-to-profiles.html#OS) could involve controlling profile activation through the `os` attribute. **However, it seems tricky to activate the same profile for both Mac and Linux simultaneously.**
**In order to reuse the configuration of the `maven-antrun-plugin`**, I'm considering setting up the `download-swagger-ui` configuration in the root pom's `pluginManagement`. In this way, it can be reused for `hugegraph-dist`, see [`1534605` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/1534605ae8ab64a2d601fd3930ec19967bf8cd27).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1302577170
##########
hugegraph-dist/pom.xml:
##########
@@ -168,51 +144,162 @@
</filesets>
</configuration>
</plugin>
-
- <plugin>
- <artifactId>maven-antrun-plugin</artifactId>
- <executions>
- <execution>
- <id>download-swagger-ui</id>
- <phase>package</phase>
- <goals>
- <goal>run</goal>
- </goals>
- <configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
- </configuration>
- </execution>
- <execution>
- <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>
+
+ <pluginManagement>
Review Comment:
need `pluginManagement`?
BTW, all the CI failed now 🔎
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1302484260
##########
hugegraph-dist/pom.xml:
##########
@@ -168,51 +144,162 @@
</filesets>
</configuration>
</plugin>
-
- <plugin>
- <artifactId>maven-antrun-plugin</artifactId>
- <executions>
- <execution>
- <id>download-swagger-ui</id>
- <phase>package</phase>
- <goals>
- <goal>run</goal>
- </goals>
- <configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
- </configuration>
- </execution>
- <execution>
- <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>
+
+ <pluginManagement>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-assembly-plugin</artifactId>
+ <version>2.4</version>
+ <executions>
+ <execution>
+ <id>assembly-hugegraph</id>
+ <phase>package</phase>
+ <goals>
+ <goal>single</goal>
+ </goals>
+ <configuration>
+ <attach>false</attach>
+ <appendAssemblyId>false</appendAssemblyId>
+ <outputDirectory>${top.level.dir}
+ </outputDirectory>
+ <descriptor>
+ ${assembly.descriptor.dir}/assembly.xml
+ </descriptor>
+ <finalName>${final.name}</finalName>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-antrun-plugin</artifactId>
+ <version>1.8</version>
+ <executions>
+ <execution>
+ <id>download-swagger-ui</id>
+ <phase>prepare-package</phase>
+ <goals>
+ <goal>run</goal>
+ </goals>
+ <configuration>
+ <target>
+ <exec executable="${shell-executable}"
+ dir="${project.basedir}"
+ failonerror="false">
+ <arg line="./dist.sh"/>
+ </exec>
+ </target>
+ </configuration>
+ </execution>
+ <execution>
+ <id>install-swagger-ui</id>
+ <phase>package</phase>
+ <goals>
+ <goal>run</goal>
+ </goals>
+ <configuration>
+ <target>
+ <exec executable="cp"
+ dir="${project.basedir}"
+ failonerror="false">
+ <arg value="-r"/>
+ <arg value="swagger-ui/dist"/>
+ <arg value="../${final.name}/swagger-ui"/>
+ </exec>
+ <exec executable="rm"
+ dir="${project.basedir}"
+ failonerror="false">
+ <arg value="-rf"/>
+ <arg value="swagger-ui"/>
+ </exec>
+ </target>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </pluginManagement>
</build>
+
+ <profiles>
+ <profile>
+ <id>assembly-hugegraph</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-assembly-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
+ <activation>
+ <activeByDefault>true</activeByDefault>
+ </activation>
+ </profile>
+ <profile>
+ <id>unix-package</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-antrun-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
+ <activation>
+ <os>
+ <family>unix</family>
+ <name>Linux</name>
+ </os>
+ </activation>
+ </profile>
+ <profile>
+ <id>mac-package</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-antrun-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
+ <activation>
+ <os>
+ <family>mac</family>
+ </os>
+ </activation>
+ </profile>
+ <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>
Review Comment:
After introducing pd and store (#2265), the relevant package goals could be considered for reuse.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1299719658
##########
hugegraph-dist/pom.xml:
##########
@@ -212,6 +229,19 @@
</configuration>
</execution>
</executions>
+ <dependencies>
+ <dependency>
+ <groupId>ant-contrib</groupId>
+ <artifactId>ant-contrib</artifactId>
+ <version>1.0b3</version>
+ <exclusions>
+ <exclusion>
+ <groupId>ant</groupId>
+ <artifactId>ant</artifactId>
+ </exclusion>
+ </exclusions>
Review Comment:
> > To avoid downloading swagger-ui on non-Linux platforms, the utilization of the `if` tag from `ant-contrib` is demonstrated here.
> > An alternative [approach](https://maven.apache.org/guides/introduction/introduction-to-profiles.html#OS) could involve controlling profile activation through the `os` attribute. **However, it seems tricky to activate the same profile for both Mac and Linux simultaneously.**
> > **In order to reuse the configuration of the `maven-antrun-plugin`**, I'm considering setting up the `download-swagger-ui` configuration in the root pom's `pluginManagement`. In This way, it can be reused in `hugegraph-dist`, see [`1534605` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/1534605ae8ab64a2d601fd3930ec19967bf8cd27).
>
> nice refactor （try it）
done in [`1534605` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/1534605ae8ab64a2d601fd3930ec19967bf8cd27)
##########
hugegraph-dist/pom.xml:
##########
@@ -212,6 +229,19 @@
</configuration>
</execution>
</executions>
+ <dependencies>
+ <dependency>
+ <groupId>ant-contrib</groupId>
+ <artifactId>ant-contrib</artifactId>
+ <version>1.0b3</version>
+ <exclusions>
+ <exclusion>
+ <groupId>ant</groupId>
+ <artifactId>ant</artifactId>
+ </exclusion>
+ </exclusions>
Review Comment:
> > To avoid downloading swagger-ui on non-Linux platforms, the utilization of the `if` tag from `ant-contrib` is demonstrated here.
> > An alternative [approach](https://maven.apache.org/guides/introduction/introduction-to-profiles.html#OS) could involve controlling profile activation through the `os` attribute. **However, it seems tricky to activate the same profile for both Mac and Linux simultaneously.**
> > **In order to reuse the configuration of the `maven-antrun-plugin`**, I'm considering setting up the `download-swagger-ui` configuration in the root pom's `pluginManagement`. In This way, it can be reused in `hugegraph-dist`, see [`1534605` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/1534605ae8ab64a2d601fd3930ec19967bf8cd27).
>
> nice refactor （try it）
done in [`1534605` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/1534605ae8ab64a2d601fd3930ec19967bf8cd27)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1301391035
##########
pom.xml:
##########
@@ -453,6 +453,30 @@
<consoleOutput>true</consoleOutput>
</configuration>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-assembly-plugin</artifactId>
+ <version>2.4</version>
+ <executions>
+ <execution>
+ <id>assembly-hugegraph</id>
+ <phase>package</phase>
+ <goals>
+ <goal>single</goal>
+ </goals>
+ <configuration>
+ <attach>false</attach>
+ <appendAssemblyId>false</appendAssemblyId>
+ <outputDirectory>${top.level.dir}
+ </outputDirectory>
+ <descriptor>
+ ${assembly.descriptor.dir}/assembly.xml
+ </descriptor>
Review Comment:
As Maven operates within the context of the current module when executing goals, the paths specified here are relative to the `hugegraph-dist` directory and can correctly resolve the `assembly.descriptor.dir` property declared within `hugegraph-dist`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#issuecomment-1687850592
与 packaging 相关的 executions 顺序应该为 `assembly-hugegraph` -> `install-swagger-ui` (copy and remove) -> `tar-package`，`download-swagger-ui` 只需要保证在 `install-swagger-ui` 之前完成即可
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1302471977
##########
pom.xml:
##########
@@ -676,8 +747,118 @@
</gpgArguments>
</configuration>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>enforce-all</id>
+ <goals>
+ <goal>enforce</goal>
+ </goals>
+ <configuration>
+ <rules>
+ <DependencyConvergence/>
+ <requireJavaVersion>
+ <version>[1.8,12)</version>
+ </requireJavaVersion>
+ <requireMavenVersion>
+ <version>[3.5.0,)</version>
+ </requireMavenVersion>
+ </rules>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+ </profile>
+ <profile>
+ <id>assembly-hugegraph</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-assembly-plugin</artifactId>
+ </plugin>
</plugins>
</build>
+ <activation>
+ <file>
+ <exists>dist.sh</exists>
+ </file>
+ </activation>
+ </profile>
+ <profile>
+ <id>unix-package</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-antrun-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
+ <activation>
+ <os>
+ <family>unix</family>
+ <name>Linux</name>
Review Comment:
looks like the `family` field can only be `unix` or `mac` 🤔
##########
pom.xml:
##########
@@ -676,8 +747,118 @@
</gpgArguments>
</configuration>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>enforce-all</id>
+ <goals>
+ <goal>enforce</goal>
+ </goals>
+ <configuration>
+ <rules>
+ <DependencyConvergence/>
+ <requireJavaVersion>
+ <version>[1.8,12)</version>
+ </requireJavaVersion>
+ <requireMavenVersion>
+ <version>[3.5.0,)</version>
+ </requireMavenVersion>
+ </rules>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+ </profile>
+ <profile>
+ <id>assembly-hugegraph</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-assembly-plugin</artifactId>
+ </plugin>
</plugins>
</build>
+ <activation>
+ <file>
+ <exists>dist.sh</exists>
+ </file>
+ </activation>
+ </profile>
+ <profile>
+ <id>unix-package</id>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-antrun-plugin</artifactId>
+ </plugin>
+ </plugins>
+ </build>
+ <activation>
+ <os>
+ <family>unix</family>
+ <name>Linux</name>
Review Comment:
looks like the `family` field can only be `unix` or `mac` 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1296627290
##########
hugegraph-dist/pom.xml:
##########
@@ -212,6 +229,19 @@
</configuration>
</execution>
</executions>
+ <dependencies>
+ <dependency>
+ <groupId>ant-contrib</groupId>
+ <artifactId>ant-contrib</artifactId>
+ <version>1.0b3</version>
+ <exclusions>
+ <exclusion>
+ <groupId>ant</groupId>
+ <artifactId>ant</artifactId>
+ </exclusion>
+ </exclusions>
Review Comment:
@imbajin By the way, what does the subtask imply here? Is there a need to support downloading swagger-ui on Windows in THIS PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: build(dist): improve download-swagger-ui in hugegraph-dist
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1292636682
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
@imbajin `curl` has been checked in [`3ae4aac` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/3ae4aacd33bcd9ee7d3913fd88683201ea4272f1#diff-5cb24cae8e78777b5af60640cc24189b37905e8d9229fcdc25298ce185caca88R190)
If there is no outside network, how can maven download dependencies from remote repositories during the building process?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1292928956
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
@imbajin Using this snippet without having curl installed:
```bash
curl --version >/dev/null 2>&1 || { echo 'WARN: Please install `curl` first'; exit; }
```
Does the following result meet your expectations?
Similar outcomes can be expected in the case of network abnormalities.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1293500552
##########
hugegraph-dist/pom.xml:
##########
Review Comment:
@imbajin done in [`0822ac4` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/0822ac40b680f570c72408184f49fd742444b43b)
To avoid explicitly modifying the script using `ReplaceRegExp`, a separate script file is used only for downloading. The necessary copy and remove tasks are accomplished in the pom file using `exec`.
By setting `failonerror` to false, the swagger-ui downloading is skipped if the network/curl is broken.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin commented on a diff in pull request #2277: build(dist): improve download-swagger-ui in hugegraph-dist
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1292976004
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
> Does the following result meet your expectations?
>
> 
>
> Similar outcomes can be expected in the case of network abnormalities.
LGTM, the warning message could be "ERROR: Please install `curl` first if you need `swagger-ui`"
##########
hugegraph-dist/pom.xml:
##########
@@ -179,21 +179,42 @@
<goal>run</goal>
</goals>
<configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
+ <target>
+ <taskdef resource="net/sf/antcontrib/antcontrib.properties"
+ classpathref="maven.plugin.classpath"/>
+ <if>
Review Comment:
> Does the following result meet your expectations?
>
> 
>
> Similar outcomes can be expected in the case of network abnormalities.
LGTM, the warning message could be `ERROR: Please install `curl` first if you need `swagger-ui``
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1296625306
##########
hugegraph-dist/pom.xml:
##########
@@ -212,6 +229,19 @@
</configuration>
</execution>
</executions>
+ <dependencies>
+ <dependency>
+ <groupId>ant-contrib</groupId>
+ <artifactId>ant-contrib</artifactId>
+ <version>1.0b3</version>
+ <exclusions>
+ <exclusion>
+ <groupId>ant</groupId>
+ <artifactId>ant</artifactId>
+ </exclusion>
+ </exclusions>
Review Comment:
To avoid downloading swagger-ui on non-Linux platforms, the utilization of the `if` tag from `ant-contrib` is demonstrated here.
An alternative [approach](https://maven.apache.org/guides/introduction/introduction-to-profiles.html#OS) could involve controlling profile activation through the `os` attribute. **However, it seems tricky to activate the same profile for both Mac and Linux simultaneously.**
**In order to reuse the configuration of the `maven-antrun-plugin`**, I'm considering setting up the `download-swagger-ui` configuration in the root pom's `pluginManagement`. In This way, it can be reused in `hugegraph-dist`, see [`1534605` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/1534605ae8ab64a2d601fd3930ec19967bf8cd27).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on a diff in pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on code in PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1302632242
##########
hugegraph-dist/pom.xml:
##########
@@ -168,51 +144,162 @@
</filesets>
</configuration>
</plugin>
-
- <plugin>
- <artifactId>maven-antrun-plugin</artifactId>
- <executions>
- <execution>
- <id>download-swagger-ui</id>
- <phase>package</phase>
- <goals>
- <goal>run</goal>
- </goals>
- <configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
- </configuration>
- </execution>
- <execution>
- <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>
+
+ <pluginManagement>
Review Comment:
> need `pluginManagement`?
>
> BTW, all the CI failed now 🔎
1. for reuse the configuration of the `maven-antrun-plugin`
2. because of https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1302502303, fix in [`880d283` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/880d28358c7c79e71caba2424b70696606ecc9e7)
##########
hugegraph-dist/pom.xml:
##########
@@ -168,51 +144,162 @@
</filesets>
</configuration>
</plugin>
-
- <plugin>
- <artifactId>maven-antrun-plugin</artifactId>
- <executions>
- <execution>
- <id>download-swagger-ui</id>
- <phase>package</phase>
- <goals>
- <goal>run</goal>
- </goals>
- <configuration>
- <tasks>
- <echo file="${project.basedir}/dist.sh">
- wget --version 1>/dev/null || exit
- wget https://github.com/swagger-api/swagger-ui/archive/refs/tags/v4.15.5.tar.gz
- tar zxvf v4.15.5.tar.gz
- echo "window.onload = function() { window.ui = SwaggerUIBundle({
- url:'/openapi.json',dom_id:'#swagger-ui',deepLinking:true,layout:'StandaloneLayout',
- presets:[SwaggerUIBundle.presets.apis, SwaggerUIStandalonePreset ],
- plugins:[SwaggerUIBundle.plugins.DownloadUrl]});};" > swagger-ui-4.15.5/dist/swagger-initializer.js
- cp -r swagger-ui-4.15.5/dist ../${final.name}/swagger-ui
- </echo>
- <exec executable="${shell-executable}" dir="${project.basedir}" failonerror="true">
- <arg line="./dist.sh"/>
- </exec>
- </tasks>
- </configuration>
- </execution>
- <execution>
- <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>
+
+ <pluginManagement>
Review Comment:
> need `pluginManagement`?
>
> BTW, all the CI failed now 🔎
1. for reuse the configuration of the `maven-antrun-plugin`
2. because of https://github.com/apache/incubator-hugegraph/pull/2277#discussion_r1302502303, fix in [`880d283` (#2277)](https://github.com/apache/incubator-hugegraph/pull/2277/commits/880d28358c7c79e71caba2424b70696606ecc9e7)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] VGalaxies commented on pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies commented on PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277#issuecomment-1691419788
**TODO:** We have observed that when the `id` of 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](https://github.com/apache/incubator-hugegraph/actions/runs/5951571368?pr=2277).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph] imbajin merged pull request #2277: chore(dist): replace wget to curl to download swagger-ui
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin merged PR #2277:
URL: https://github.com/apache/incubator-hugegraph/pull/2277
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org