Bug 149071 - Rid of licenses in Update Center Descriptor
Rid of licenses in Update Center Descriptor
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Autoupdate
6.x
All All
: P1 (vote)
: 6.x
Assigned To: dlipin
issues@platform
: API, API_REVIEW_FAST
: 136255 (view as bug list)
Depends on:
Blocks: 148324
  Show dependency treegraph
 
Reported: 2008-10-03 14:31 UTC by Jiri Rechtacek
Modified: 2009-03-17 08:36 UTC (History)
6 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
first implementation drop (35.59 KB, patch)
2009-03-03 13:20 UTC, dlipin
Details | Diff
patch to the MakeUpdateDesc.java (3.65 KB, patch)
2009-03-03 15:17 UTC, dlipin
Details | Diff
Conditional using of license-in-url/license-in-content (38.69 KB, patch)
2009-03-03 18:08 UTC, dlipin
Details | Diff
patch which also sets the correct DOCTYPE and fix 'licence' typo (40.34 KB, patch)
2009-03-03 18:24 UTC, dlipin
Details | Diff
patch with update to XMLUtil.java and other minor changes (41.15 KB, patch)
2009-03-05 16:56 UTC, dlipin
Details | Diff
test that checks <license> url attribute (3.39 KB, patch)
2009-03-11 13:35 UTC, dlipin
Details | Diff
another version of test with having cddl.txt in classpath (not in Internet) (20.70 KB, patch)
2009-03-12 11:33 UTC, dlipin
Details | Diff
the same as test2.diff plus removed createUpdateLicense(String,Stirng,URL) in UpdateLicense. Note: no spec version change. (21.59 KB, patch)
2009-03-12 13:47 UTC, dlipin
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiri Rechtacek 2008-10-03 14:31:39 UTC
The licenses take more than 80% size of Update Center Descriptor. It causes OutOfMemoryException while parsing it
sometimes (e.g. issue 148324). As same licenses are stored in NBM metadata although. I propose to read them from NBM
metadata instead of UC Descriptor and save download size, time of downloading UCs and memory while parsing them.
The current scenario of install plugins:
1. choose plugins
2. accept licenses
3. download plugins
4. verify download
5. install plugins.
The new proposed scenario will look like
1. choose plugins
2. download plugins
3. accept licenses as a part verification of download
4. install plugins
Comment 1 Jiri Rechtacek 2008-10-03 14:34:26 UTC
It means change of DTD also. Should be tracked as a API change.
Comment 2 Jesse Glick 2008-10-03 16:55:43 UTC
I'm not sure a DTD change is required. The 'license' attribute is optional, as are later <license> elements. Simply

1. Change AU to ignore any licenses specified in the descriptor, showing only licenses from the actual NBMs after
download. (This is not incompatible, as you were always required to put the license in the NBM even if it was duplicated
in the descriptor.)

2. Change <makeupdatedesc> to omit licenses. (Or for compatibility: add an option to omit them, and turn that option on
in common.xml when it can be determined that the target platform contains the version of AU which has the fix of #1.)
Comment 3 Jiri Rechtacek 2008-10-16 17:05:35 UTC
Increasing to P1 because some duplicates of OutOfMemoryError: Java heap space (issue 148324)
Comment 4 Jiri Rechtacek 2008-10-17 07:47:05 UTC
*** Issue 136255 has been marked as a duplicate of this issue. ***
Comment 5 Antonin Nebuzelsky 2008-11-14 15:31:49 UTC
Reassigning to the new "autoupdate/*" owner dlipin.
Comment 6 dlipin 2008-11-19 12:47:10 UTC
Jirka, how does "run in background" functionality fits in the proposed scenario?
From one side we can`t just silently accept all the licenses and from the other side it is not good thing to return 
back from background task by forcing showing the license panel. It is certainly possible to show the baloon with the 
"Click here to read and accept license" message but that does not right to me from the first view...
Comment 7 Jiri Rechtacek 2008-11-19 12:59:59 UTC
Dima, you are right. It could be a problem. The proposed balloon-like solution looks good to me in my point of view but
it should be confirmed by UI team. Ccing Jano for his UI opinion.
Jano, could you look on this RFE or ask someone else who can give UI feedback? Thanks
Comment 8 dlipin 2008-11-19 15:17:34 UTC
Just an idea.

Since each license in the resulting catalog.xml has its "name" like EB738DDF (it is the hash, right?), why not store 
all the licenses as the separate files?
Say,
/catalog.xml
/catalog.xml.gz
/licenses/
/licenses/EB738DDF (maybe gzipped)
/licenses/1E0874BF (maybe gzipped)

or
/catalog.xml
/catalog.xml.gz
/licenses.zip <- yes, that`s the zip archive. Or maybe .jar to have the ability to sign it...
/licenses.zip/EB738DDF
/licenses.zip/1E0874BF

Comment 9 Jiri Rechtacek 2008-11-19 15:59:59 UTC
It could help to decrease parsing time and save some memory on heap, and will avoid the biggest related problem (issue
148324), in additional UI can left w/o change which is benefit for implementation.
On other hand, it doesn't reduce download size because licenses left both in catalog and in NBM/Info.xml. Moreover,
storing licenses on both places and keeping it in sync could be error prone.
Comment 10 dlipin 2009-02-27 11:12:42 UTC
The proposed scenario has not been accepted by xDesign (Jano). Instead of that he suggested the following:

> select -> download license -> license -> download plugins and install
> The installer downloads licenses of selected plugins, which I guess should be pretty fast. 
> Then the installer downloads and install plugins in a single step. 
> This way the download and install step can run in background. 

But, as Jirka says:
> Well but it requires changes in Update Center Descriptor DTD (a API change) and changes in files layout on server 
side. 
> Separate each license into a separate file beside NBM and link on it from UC Descriptor.


To fix the possible OOM exception I can also propose the following thing: don`t store all licenses in the memory but 
dump them to disk just after reading it during the catalog parsing.

In a few words, create 2 methods in AutoupdateCatalogCache :
void writeLicense(String name, String content) {
   write (contents) to file new File(cache, name);
}
String getLicense(String name) {
  read contents of file new File(cache, name);
}

and use them in UpdateLicenseImpl.java  in the following way:
public String getAgreement () {
       return AutoupdateCatalogCache.getDefault().getLicense(name);
}  public void setAgreement (String content) {
       AutoupdateCatalogCache.getDefault().writeLicense(name,content);
}

It won`t help to reduce the catalog size and likely will be a little bit slower (since the hard drive will be accessed 
instead of memory) but it will allow to avoid keeping all the licenses in memory. The licenses, needed to show during 
the update/installation, will be loaded - but the rest are not required to be kept in memory. 

Regarding Jano/Jirka`s proposal to store each license separately and refering it from the catalog I have a few concerns 
(hard to say whether they are important or not):
1) it can give more pain to keep in consistency the overall bunch of catalog-plugins-licenses files in comparison with 
catalog-plugins files.
2) downloading licenses can be fast and can be not such. It depends on the number of plugins for installation/update 
and the connection speed. Certainly, we can process them properly by e.g. it can be handled by adding the progress bar 
on the panel. 
Comment 11 dlipin 2009-03-03 10:51:28 UTC
The proposed change regarding the licenses is the following:
1) add the "url" attribute to the <license> element in the dtd and make it the *2_6.dtd
diff -r 4670bfefcce8 autoupdate.services/libsrc/org/netbeans/updater/resources/autoupdate-catalog-2_5.dtd
--- a/autoupdate.services/libsrc/org/netbeans/updater/resources/autoupdate-catalog-2_5.dtd	Mon Mar 02 15:11:25 
2009 -0500
+++ b/autoupdate.services/libsrc/org/netbeans/updater/resources/autoupdate-catalog-2_5.dtd	Tue Mar 03 13:39:42 
2009 +0300
@@ -63,4 +63,5 @@
                  OpenIDE-Module-Long-Description CDATA #IMPLIED>
 
 <!ELEMENT license (#PCDATA)>
-<!ATTLIST license name CDATA #REQUIRED>
+<!ATTLIST license name CDATA #REQUIRED
+                  url  CDATA #IMPLIED>

2) do not change the autoupdate-info-*.dtd
3) store licenses as <cluster>/<module-name>.license (e.g. java2/org-netbeans-modules-i18n.license) - modify nbbuild/
antsrc/org/netbeans/nbbuild/MakeUpdateDesc.java to do that
4) refer to that licenses via relative path (like we do with "distribution" attribute) e.g.
<license name="DCEB978B" url="java2/org-netbeans-modules-i18n.license"/>
5) Create a new method in autoupdate.services/src/org/netbeans/spi/autoupdate/UpdateLicense.java and use it from the 
AutoupdateCatalogParser
    public static final UpdateLicense createUpdateLicense (String licenseName, String agreement, URL agreementUrl) {
        return new UpdateLicense (new UpdateLicenseImpl (licenseName, agreement, agreementUrl));
    }
6) Cache licenses by name in <userdir>/var/cache/catalogcahe/licenses/<name>, e.g. ~/.netbeans/6.7/var/cache/
catalogcache/licenses/DCEB978B

Any suggestions/objections/questions ?
Thanks,
Dmitry
Comment 12 dlipin 2009-03-03 13:20:01 UTC
Created attachment 77625 [details]
first implementation drop
Comment 13 dlipin 2009-03-03 13:24:13 UTC
the attached patch is the first implementation drop of proposed change. It does not yet include the changes required to 
be done in nbbuild/antsrc. Some tweaks still need to be done in dealing with licenses in the InstallWizardIterator (due 
to the fact the obtaining licenses is not instant and can take some time).
Comment 14 Jesse Glick 2009-03-03 14:04:31 UTC
Licenses should be named by hash, since they are usually redundant. And don't use the top level of the cluster. Suggest e.g.

<license name="DCEB978B" url="java2/licenses/DCEB978B.license"/>
Comment 15 dlipin 2009-03-03 15:16:15 UTC
agree. the only thing is the hash (and this the license) is uniq among the overall catalog not only in cluster.
Thus I propose to have
<license name="DCEB978B" url="licenses/DCEB978B.license"/>

The patch is attached. Not sure what is the best way to set DOCTYPE to autoupdate-catalog-2_6.dtd instead of autoupdate-
catalog-2_5.dtd. Maybe by a parametr passed to the ant task?
Comment 16 dlipin 2009-03-03 15:17:25 UTC
Created attachment 77635 [details]
patch to the MakeUpdateDesc.java
Comment 17 Jiri Rechtacek 2009-03-03 15:32:07 UTC
The proposal looks good. I agree with the proposed change. Thanks
Comment 18 Jesse Glick 2009-03-03 16:03:47 UTC
The selection of the new DTD needs to be conditional - when the build harness is running against NB 6.5 or earlier, it
should continue to generate the old format. Adding Richard to CC therefore.
Comment 19 dlipin 2009-03-03 18:08:13 UTC
Created attachment 77660 [details]
Conditional using of license-in-url/license-in-content
Comment 20 Jesse Glick 2009-03-03 18:18:05 UTC
s/licence/license/ in suite.xml.

Looking closer. MakeUpdateDesc needs to adjust the public & system IDs it generates according to the value of
useLicenseUrl. (Last I checked this task automatically validates whatever it generates according to the declared DTD,
which should help enforce correctness.)
Comment 21 dlipin 2009-03-03 18:22:57 UTC
See the proposed patch (not deeply tested by me).

It adds useLicenseUrl parameter in MakeUpdateDesc task. 
In nbbuild/build.xml it is controlled by ${catalog.use.license.url} with default "true" value.

In apisupport.harness/release/suite.xml it makes a decision on whether the *catalog_2_6.dtd is availalble in updater or 
not. Can be explicitely set using ${use.licence.url.in.catalog} property.

I`m open for any suggestions on how make/handle it in a better way.

The patch also contains some changes in UI and license caching. In case license is not available at the specified URL 
(e.g. UC is down), then it is treated as empty but deleted on platform/IDE exit. Some issues with (not)showing the 
license panel (in case all licenses were already accepted) should be also resolved by this patch.
Comment 22 dlipin 2009-03-03 18:24:12 UTC
Created attachment 77661 [details]
patch which also sets the correct DOCTYPE and fix 'licence' typo
Comment 23 dlipin 2009-03-03 18:26:58 UTC
Jesse, thanks for the notes - should be fixed in autoupdate_drop3.diff.
Not sure that I`ve got your idea about validation - is it another task for that? o.n.nbbuild.VerifyUpdateCenter ?
Comment 24 Jesse Glick 2009-03-03 18:43:50 UTC
See MakeNBM.validateAgainstAUDTDs in MakeUpdateDesc.java. It should be rejecting attempts to use <license url="...">
with the 1.5 DTD.
Comment 25 dlipin 2009-03-04 13:16:08 UTC
I`ve checked that this is already works as expected. 
I`ve tried to use 2_5 catalog DTD (instead of 2_6) in MakeUpdateDesc with <license url="..."/> and 
MakeNBM.validateAgainstAUDTDs verification failed (as expected).

So... what else should be done to push this change?
Any spec version increase required? for which modules? 
Should the new DTD be uploaded to http://www.netbeans.org/dtds/autoupdate-catalog-2_6.dtd? who can do that?
Should any arch.xml be updated?

Should we consider gzip`ed licenses support in the catalog? i.e. have
<license url="licenses/aaabbbcc.license.gzip" name="aaabbbcc"/> or
<license url="licenses/aaabbbcc.license.gzip" gzip="true" name="aaabbbcc"/>
Jesse/Jirka - from your POV - is it worth doing or sounds reduntant?
Comment 26 Jesse Glick 2009-03-04 17:45:49 UTC
The new DTD should be uploaded. Just

  cvs co www/www/dtds

to get the folder into which you can store it (update 'catalog' too). Or simply send it to me to do, if you prefer.


autoupdate.services/libsrc/org/netbeans/updater/XMLUtil.java should be updated with the new location. Otherwise you will
be contacting the network when you unpack an NBM, which is bad.
Comment 27 dlipin 2009-03-05 16:55:33 UTC
The last (hopefully) version of patch is attached, I`ll push it (together with dtd commit to cvs) tomorrow after the 
full local build.
Jesse - thanks for the reminder about XMLUtil.java - I`ve forgot about it really.

I`ve increased spec.version for autoupdate.ui and autoupdate.services only.
Comment 28 dlipin 2009-03-05 16:56:59 UTC
Created attachment 77776 [details]
patch with update to XMLUtil.java and other minor changes
Comment 29 dlipin 2009-03-06 10:02:12 UTC
Fixed.

core-main#2de5072bf5e7

D:\space\dtds>cvs add autoupdate-catalog-2_6.dtd
cvs server: scheduling file `autoupdate-catalog-2_6.dtd' for addition
cvs server: use 'cvs commit' to add this file permanently
D:\space\dtds>cvs ci -m "Add new catalog DTD due to fix Issue #149071" autoupdate-catalog-2_6.dtd
RCS file: /cvs/www/www/dtds/autoupdate-catalog-2_6.dtd,v
done
Checking in autoupdate-catalog-2_6.dtd;
/cvs/www/www/dtds/autoupdate-catalog-2_6.dtd,v  <--  autoupdate-catalog-2_6.dtd
initial revision: 1.1
done
Comment 30 Quality Engineering 2009-03-07 09:47:10 UTC
Integrated into 'main-golden', will be available in build *200903070353* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/2de5072bf5e7
User: Dmitry Lipin <dlipin@netbeans.org>
Log: Issue #149071 Rid of licenses in Update Center Descriptor
Comment 31 Jaroslav Tulach 2009-03-10 15:29:01 UTC
It is better to ask for review, before the change is integrated:
http://openide.netbeans.org/tutorial/reviews/


Y01 createUpdateLicense in 2de5072bf5e7 is missing some real javadoc and @since tag.
Y02 slight test, at least for logic of createUpdateLicense, or the way XML is parsed would be nice contribution for 
the future maintainers of this API too

Comment 32 dlipin 2009-03-11 13:34:46 UTC
> Y01 createUpdateLicense in 2de5072bf5e7 is missing some real javadoc and @since tag.
I`ve re-checked the code and it looks like that I`ve introduce this method by have never used it.
So it worth removing at all.

> Y02 slight test, at least for logic of createUpdateLicense, or the way XML is parsed would be nice contribution for 
the future maintainers of this API too
OK, attached.
Comment 33 dlipin 2009-03-11 13:35:35 UTC
Created attachment 78048 [details]
test that checks <license> url attribute
Comment 34 dlipin 2009-03-12 11:33:51 UTC
Created attachment 78114 [details]
another version of test with having cddl.txt in classpath (not in Internet)
Comment 35 dlipin 2009-03-12 13:47:24 UTC
Created attachment 78116 [details]
the same as test2.diff plus removed createUpdateLicense(String,Stirng,URL) in UpdateLicense. Note: no spec version change.
Comment 36 dlipin 2009-03-13 14:07:39 UTC
core-main #90eb80ae3716
Comment 37 Quality Engineering 2009-03-17 08:36:13 UTC
Integrated into 'main-golden', will be available in build *200903170201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/90eb80ae3716
User: Dmitry Lipin <dlipin@netbeans.org>
Log: Issue #149071 Rid of licenses in Update Center Descriptor (making changes in accordance with api review)


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo