This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
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
It means change of DTD also. Should be tracked as a API change.
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.)
Increasing to P1 because some duplicates of OutOfMemoryError: Java heap space (issue 148324)
*** Issue 136255 has been marked as a duplicate of this issue. ***
Reassigning to the new "autoupdate/*" owner dlipin.
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...
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
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
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.
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.
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
Created attachment 77625 [details] first implementation drop
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).
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"/>
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?
Created attachment 77635 [details] patch to the MakeUpdateDesc.java
The proposal looks good. I agree with the proposed change. Thanks
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.
Created attachment 77660 [details] Conditional using of license-in-url/license-in-content
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.)
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.
Created attachment 77661 [details] patch which also sets the correct DOCTYPE and fix 'licence' typo
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 ?
See MakeNBM.validateAgainstAUDTDs in MakeUpdateDesc.java. It should be rejecting attempts to use <license url="..."> with the 1.5 DTD.
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?
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.
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.
Created attachment 77776 [details] patch with update to XMLUtil.java and other minor changes
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
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
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
> 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.
Created attachment 78048 [details] test that checks <license> url attribute
Created attachment 78114 [details] another version of test with having cddl.txt in classpath (not in Internet)
Created attachment 78116 [details] the same as test2.diff plus removed createUpdateLicense(String,Stirng,URL) in UpdateLicense. Note: no spec version change.
core-main #90eb80ae3716
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)