Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide docker-web project for gradle, restructure web static content #1694
Provide docker-web project for gradle, restructure web static content #1694
Changes from all commits
c5b74e9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The jar name is different than before.
deephaven-web-client-ide-0.8.0-js.jar
vsweb-0.8.0.jar
. Since it was a under the guise of a java project before, it inheritedbuildSrc/src/main/groovy/io.deephaven.java-conventions.gradle
which includes settingarchivesBaseName
. The easy fix is to set. Long term, we should probably have
base-conventions.gradle
separated out.Do we need/want the
-js
classifier on the jar name?Upon my first build of
web:ideClientJsJar
, I was seeing extra content. It may have been old gradle directories that were being passively picked up. Was a bit strange. After a clean it went away.Here are the
jar tf
for the builds:You can see the first web build is about twice as big as web2.
And as far as content, I see that there are some different prefixes now.
For example:
dhapi
->jsapi
anddhide
->ide
, that's correct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we care? I'm not sure either way just yet. This jar is being created only so that it can be on the classpath of the eventual jetty-based executable, so that content can be served off of its classpath.
If we publish the web ui's static content as a jar to central I'm sure we care. Otherwise I'm uncertain, will be guided by you.
No, this project does not produce java that is useful downstream to anyone (at this time?)
--
Probably your uncleaned version had leftover UI stuff copied into it - I've seen that a lot recently where gradle is over-confident in what is correctly already built and what isn't. 99% the "fault" is actually mine, but it is very hard to reliably use when it is so hard to hold the tool correctly. I'll take another pass at checking the before/after of this.
--
"jsapi" and "ide" are the desired paths that the user sees in the browser. We can probably rewrite the "dhapi" and "dhide" internally, but these path changes are consistent with the build before my patch. If you'd like, I'll address that now, your call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do care about about the jar name prefix -
deephaven-<x>
, I don't care about the classifier being appended.Please set the archiveBaseName to
deephaven-web
(or something similar if you think it makes more sense), but put a reference in the code to #1701