Re: RFR 8193255: Root Certificates should be stored in text format and assembled at build time
> On May 31, 2019, at 11:15 PM, Sean Mullan wrote: > > On 5/30/19 8:49 PM, Weijun Wang wrote: >> Sure. How many info do you want to see? >> I can prepend `keytool -printcert` but that's too much. At least I think the >> extensions part is not needed. Also, I don't wish people reading the >> fingerprint inside as genuine and does not calculate it from the cert itself. >> So, I'm thinking of >> Owner: CN=XRamp Global Certification Authority, O=XRamp Security Services >> Inc, OU=www.xrampsecurity.com, C=US >> Issuer: CN=XRamp Global Certification Authority, O=XRamp Security Services >> Inc, OU=www.xrampsecurity.com, C=US >> Serial number: 50946cec18ead59c4dd597ef758fa0ad >> Valid from: 1 Nov 2004 17:14:04 GMT until: 1 Jan 2035 05:37:19 GMT >> Signature algorithm name: SHA1withRSA >> Subject Public Key Algorithm: 2048-bit RSA key >> Version: 3 >> Is that OK? > > This is good. Did you use keytool to emit those fields? Yes. > It might make sense to add a brief README in this directory with instructions > or a code snippet so that the next time we add a cert we know what to include > at the top for consistency. OK, so it might contain: -START- Each file in this directory (except for this README) contains a CA certificate in PEM format. It can be generated with keytool -J-Duser.timezone=GMT -printcert -file ca.cert | sed -n '1,4p;8,10p' keytool -printcert -file ca.cert -rfc Please note the textual comment is just a suggestion and not arbitrary. After any change in this directory, please remember to update the content of `test/jdk/sun/security/lib/cacerts/VerifyCACerts.java` as well. -END- And I'll need to skip this README file in the build tool. --Max > > Thanks, > Sean > >> Thanks, >> Max >> p.s. `keytool -printcert` shows validity in local timezone. Does not look >> good to me. >>> On May 31, 2019, at 6:51 AM, Sean Mullan wrote: >>> >>> One suggestion is to put a printable form of the contents of the >>> certificate at the top of each of the PEM files. It would be nice as a >>> quick-look to see what is in the certificate. Of course, you can also use >>> keytool -printcert to do that, but if I am just perusing the source code >>> via a browser or something like that, it would be nice to not have to do >>> that. >>> >>> --Sean >>> >>> On 5/30/19 9:01 AM, Weijun Wang wrote: Please take a review at http://cr.openjdk.java.net/~weijun/8193255/webrev.00/ Please pay attention to the 1st 3 and the last 2 files. Others are PEM files for all certs inside the original cacerts. There is one thing I cannot get correct. If I update the GenerateCacerts.java file and rerun make, the cacerts file is unchanged. I thought the following line $(GENDATA_CACERTS): $(BUILD_TOOLS) $(GENDATA_CACERTS_SRC) means when when the tool is changed, GENDATA_CACERTS will be called. Thanks, Max
Re: RFR: docs JDK-8225134: Update man-page files
Bringing these pages to more recent version is better than the current so outdated version even it's just a snapshot. The list of new pages look okay (but I didn't validate the content.) Mandy On 5/31/19 2:02 PM, Jonathan Gibbons wrote: Please review a change to refresh the content of the man pages (*.1 files) in the main repo, to contain essentially the exact same content as found in the Oracle JDK man pages. Going forward, the intent is to bulk-update these pages towards the end of each release cycle, until we can eventually open source the underlying files from which the man pages are derived, at which time these files will be deleted. This email is to build-dev@ojn, even though there are no Makefile changes, because that list includes the folk mostly likely to make direct use of these files. It is also cc jdk-dev@ojn so that the OpenJDK community is generally aware of this change. Note, although the current man pages share a history with the previous versions of these files, the transformations that have occurred between then and now make it all but impossible to compare the versions side by side with diff. For those folk wanting to review the content, it is best to look at the "New" or "Raw" files in the webrev. -- Jon JBS: https://bugs.openjdk.java.net/browse/JDK-8225134 Webrev: http://cr.openjdk.java.net/~jjg/8225134/webrev.00/webrev/
Re: RFR: docs JDK-8225134: Update man-page files
Looks good to me. /Erik On 2019-05-31 14:02, Jonathan Gibbons wrote: Please review a change to refresh the content of the man pages (*.1 files) in the main repo, to contain essentially the exact same content as found in the Oracle JDK man pages. Going forward, the intent is to bulk-update these pages towards the end of each release cycle, until we can eventually open source the underlying files from which the man pages are derived, at which time these files will be deleted. This email is to build-dev@ojn, even though there are no Makefile changes, because that list includes the folk mostly likely to make direct use of these files. It is also cc jdk-dev@ojn so that the OpenJDK community is generally aware of this change. Note, although the current man pages share a history with the previous versions of these files, the transformations that have occurred between then and now make it all but impossible to compare the versions side by side with diff. For those folk wanting to review the content, it is best to look at the "New" or "Raw" files in the webrev. -- Jon JBS: https://bugs.openjdk.java.net/browse/JDK-8225134 Webrev: http://cr.openjdk.java.net/~jjg/8225134/webrev.00/webrev/
RFR: docs JDK-8225134: Update man-page files
Please review a change to refresh the content of the man pages (*.1 files) in the main repo, to contain essentially the exact same content as found in the Oracle JDK man pages. Going forward, the intent is to bulk-update these pages towards the end of each release cycle, until we can eventually open source the underlying files from which the man pages are derived, at which time these files will be deleted. This email is to build-dev@ojn, even though there are no Makefile changes, because that list includes the folk mostly likely to make direct use of these files. It is also cc jdk-dev@ojn so that the OpenJDK community is generally aware of this change. Note, although the current man pages share a history with the previous versions of these files, the transformations that have occurred between then and now make it all but impossible to compare the versions side by side with diff. For those folk wanting to review the content, it is best to look at the "New" or "Raw" files in the webrev. -- Jon JBS: https://bugs.openjdk.java.net/browse/JDK-8225134 Webrev: http://cr.openjdk.java.net/~jjg/8225134/webrev.00/webrev/
Re: RFR: 8224851: AArch64: fix warnings and errors with Clang and GCC 8.3
> On May 31, 2019, at 4:46 PM, Kim Barrett wrote: > >> On May 31, 2019, at 12:11 AM, Nick Gasson wrote: >> >> /home/nicgas01/jdk/src/hotspot/share/gc/g1/g1PageBasedVirtualSpace.cpp:259:34: >> note: in instantiation of function template specialization >> 'Atomic::add' requested here >> char* touch_addr = Atomic::add(actual_chunk_size, &_cur_addr) - >> actual_chunk_size; >>^ > […] > If I'm reading the gcc documentation for __sync_add_and_fetch > correctly, I think the following (completely untested) should work, > and won't cause Andrew to (I think rightly) complain about > type-punning reinterpret_casts. > > Though I wonder if this is a clang bug. (See below.) > > […] > The bug description says this warning only arises with clang. It > seems like clang is overdoing that as-if, and treating it literally as > uintptr_t all the way through to the result type. Or don’t change the code at all here, and say that clang is not (currently) a supported compiler for this platform.
Re: RFR: 8224851: AArch64: fix warnings and errors with Clang and GCC 8.3
> On May 31, 2019, at 12:11 AM, Nick Gasson wrote:
>>> /home/nicgas01/jdk/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp:43:39:
>>> error: cannot initialize a parameter of type 'char *' with an lvalue of
>>> type 'unsigned long'
>>>return __sync_add_and_fetch(dest, add_value);
>>> ^
>>>
>>> If the add_and_fetch template is instantiated with I=integer and
>>> D=pointer then we need an explicit cast here.
>>
>> Please don't. Please have a look at where this happens and fix the
>> types there, as appropriate. What do other ports do?
>
> It's instantiated here:
>
> /home/nicgas01/jdk/src/hotspot/share/gc/g1/g1PageBasedVirtualSpace.cpp:259:34:
> note: in instantiation of function template specialization
> 'Atomic::add' requested here
> char* touch_addr = Atomic::add(actual_chunk_size, &_cur_addr) -
> actual_chunk_size;
> ^
>
> We're adding an unsigned long to a char* which seems legitimate.
>
> The other ports except Arm32 and Zero use inline assembly so don't have
> problems with mismatched types. Zero uses __sync_fetch_and_add so would
> hit this too if it used G1.
>
> I'm not sure how to resolve this. We could add a specialisation of
> PlatformAdd for byte_size=8 and cast to (u)intptr_t?
>
> template<>
> struct Atomic::PlatformAdd<8>
> : Atomic::AddAndFetch >
> {
> template
> D add_and_fetch(I add_value, D volatile* dest, atomic_memory_order order)
> const {
>STATIC_ASSERT(8 == sizeof(I));
>STATIC_ASSERT(8 == sizeof(D));
>
>typedef typename Conditional::value, intptr_t,
> uintptr_t>::type PI;
>
>PI res = __sync_add_and_fetch(reinterpret_cast(dest),
> static_cast(add_value));
>return reinterpret_cast(res);
> }
> }
>
> I think this is safe.
If I'm reading the gcc documentation for __sync_add_and_fetch
correctly, I think the following (completely untested) should work,
and won't cause Andrew to (I think rightly) complain about
type-punning reinterpret_casts.
Though I wonder if this is a clang bug. (See below.)
template
struct Atomic::PlatformAdd
: Atomic::AddAndFetch >
{
template
D add_and_fetch(I add_value, D volatile* dest, atomic_memory_order order)
const {
// Cast to work around clang pointer arithmetic bug.
return PrimitiveConversions::cast(__sync_add_and_fetch(dest, add_value));
}
}
The key bit from the documentation is "Operations on pointer arguments
are performed as if the operands were of the uintptr_t type."
The bug description says this warning only arises with clang. It
seems like clang is overdoing that as-if, and treating it literally as
uintptr_t all the way through to the result type.
The documentation also says "That is, they are not scaled by the size
of the type to which the pointer points." Any needed scaling of
add_value has already been done in the calling Atomic::AddImpl.
Re: RFR: JDK-8225140: Build fails if directory contains 'unix'
Erik: This patch changes the filter mechanics to only apply to the path relative to the workspace root. I also changed the filter token from "unix" to "/unix/" to avoid the risk of matching substrings in directories in the future. Bug: https://bugs.openjdk.java.net/browse/JDK-8225140 Webrev: http://cr.openjdk.java.net/~erikj/8225140/webrev.01/index.html Looks good. Tim
RFR: JDK-8225140: Build fails if directory contains 'unix'
If the workspace directory (or any parent) contains the word "unix", the build fails in java.desktop-libs. There are several libraries here that use the EXCLUDE_SRC_FILTER option of SetupJdkLibrary to filter away the src/java.desktop/unix/lib* dirs. This option incorrectly includes the whole absolute path of each source file when applying the filter, so when the workspace dir contains "unix", these libraries get no source files at all. This patch changes the filter mechanics to only apply to the path relative to the workspace root. I also changed the filter token from "unix" to "/unix/" to avoid the risk of matching substrings in directories in the future. Bug: https://bugs.openjdk.java.net/browse/JDK-8225140 Webrev: http://cr.openjdk.java.net/~erikj/8225140/webrev.01/index.html /Erik
Re: RFR 8193255: Root Certificates should be stored in text format and assembled at build time
This looks good to me, and thanks for fixing the other instances of BUILD_TOOLS_JDK! /Erik On 2019-05-30 20:00, Weijun Wang wrote: New webrev at https://cr.openjdk.java.net/~weijun/8193255/webrev.01 Changes: 1. Textual info at the beginning of each cert 2. Makefile: indentation, BUILD_TOOLS_JDK, MakeTargetDir, files instead of dir Thanks, Max On May 31, 2019, at 1:34 AM, Erik Joelsson wrote: On 2019-05-30 08:32, Weijun Wang wrote: On May 30, 2019, at 10:01 PM, Erik Joelsson wrote: In my experience, using directories for dependencies in make does not work well. Since all the files in make/data/cacerts are in a flat structure, I would recommend expressing the prerequisites as: $(wildcard $(GENDATA_CACERTS_SRC)/*) This will not cover the case where a file is removed, but that case is rarely handled well in make based build systems. But in my experiment, using the directory name does detect the file removal. It believe that worked well on your machine, but directory timestamp updates are file system dependent. I'm not sure we can count on all filesystems to accurately reflect time stamps based on file modification. It's also possible that an OS would touch directory timestamps for other reasons, which should not affect the build. I haven't tried having source directories as prerequisites before, so I simply don't know how reliable it is. My experience is rather with directories as targets, which certainly doesn't work. If you verified that it worked as expected on all supported OSes, I would be less worried. Or, can I list *both* the files and the directory to get maximum awareness? The directory modification time will usually not change when a file in it is modified, only when adding or removing files (and possibly some other operations), so adding the files is certainly a must. If you go with both, then I would just be worried about potential false positives. /Erik
Re: RFR: 8223678: Add Visual Studio Code workspace generation support (for native code)
Hello Robin, On 2019-05-31 05:26, Robin Westberg wrote: Hi Erik, On 29 May 2019, at 17:22, Erik Joelsson wrote: Thanks, looks good! Thanks for reviewing! Unfortunately I had to do a few more general changes to get the “ccls" indexer working properly as well, hope you don’t mind taking another look: Webrevs: - Full: http://cr.openjdk.java.net/~rwestberg/8223678/webrev.03/ - Inc: http://cr.openjdk.java.net/~rwestberg/8223678/webrev.02-03/ Looks good! I sure appreciate adding tests when modifying the hairier utility macros. /Erik Best regards, Robin /Erik On 2019-05-29 06:52, Robin Westberg wrote: Hi Erik, Thanks for taking a look! On 28 May 2019, at 15:52, Erik Joelsson wrote: Hello Robin, Looks good. Adding of ide.md is very nice, but could you try to format it like testing.md and building.md in regards to line lengths? I believe we are trying for 80 chars in those to make them read reasonably in a standard editor. Sure, did a bit of reflowing and updated some formatting in order to look more like the other .md files. Webrevs: - Full: https://cr.openjdk.java.net/~rwestberg/8223678/webrev.02/ - Inc: https://cr.openjdk.java.net/~rwestberg/8223678/webrev.01-02/ Best regards, Robin /Erik On 2019-05-27 09:03, Robin Westberg wrote: Hi all, Please review this change that adds build system support for generating a Visual Studio Code workspace configured for working with the JDK native code. It configures the default C/C++ IntelliSense Engine to allow code completion/navigation and similar features. It also configures two executable targets (gtestLauncher and java) that can be built and debugged from the IDE. The main target is "make vscode-project”, additional information can be found in doc/ide.[md|html]. Issue: https://bugs.openjdk.java.net/browse/JDK-8223678 Webrev: https://cr.openjdk.java.net/~rwestberg/8223678/webrev.01/ Testing: Manual testing on Linux, MacOS and Windows Thanks Erik Joelsson for taking a look at an earlier version of it! Best regards, Robin
Re: RFR 8193255: Root Certificates should be stored in text format and assembled at build time
On 5/30/19 8:49 PM, Weijun Wang wrote: Sure. How many info do you want to see? I can prepend `keytool -printcert` but that's too much. At least I think the extensions part is not needed. Also, I don't wish people reading the fingerprint inside as genuine and does not calculate it from the cert itself. So, I'm thinking of Owner: CN=XRamp Global Certification Authority, O=XRamp Security Services Inc, OU=www.xrampsecurity.com, C=US Issuer: CN=XRamp Global Certification Authority, O=XRamp Security Services Inc, OU=www.xrampsecurity.com, C=US Serial number: 50946cec18ead59c4dd597ef758fa0ad Valid from: 1 Nov 2004 17:14:04 GMT until: 1 Jan 2035 05:37:19 GMT Signature algorithm name: SHA1withRSA Subject Public Key Algorithm: 2048-bit RSA key Version: 3 Is that OK? This is good. Did you use keytool to emit those fields? It might make sense to add a brief README in this directory with instructions or a code snippet so that the next time we add a cert we know what to include at the top for consistency. Thanks, Sean Thanks, Max p.s. `keytool -printcert` shows validity in local timezone. Does not look good to me. On May 31, 2019, at 6:51 AM, Sean Mullan wrote: One suggestion is to put a printable form of the contents of the certificate at the top of each of the PEM files. It would be nice as a quick-look to see what is in the certificate. Of course, you can also use keytool -printcert to do that, but if I am just perusing the source code via a browser or something like that, it would be nice to not have to do that. --Sean On 5/30/19 9:01 AM, Weijun Wang wrote: Please take a review at http://cr.openjdk.java.net/~weijun/8193255/webrev.00/ Please pay attention to the 1st 3 and the last 2 files. Others are PEM files for all certs inside the original cacerts. There is one thing I cannot get correct. If I update the GenerateCacerts.java file and rerun make, the cacerts file is unchanged. I thought the following line $(GENDATA_CACERTS): $(BUILD_TOOLS) $(GENDATA_CACERTS_SRC) means when when the tool is changed, GENDATA_CACERTS will be called. Thanks, Max
Re: RFR: 8223678: Add Visual Studio Code workspace generation support (for native code)
Hi Volker, > On 29 May 2019, at 16:01, Volker Simonis wrote: > > On Wed, May 29, 2019 at 3:43 PM Robin Westberg > wrote: >> >> Hi Volker, >> >>> On 28 May 2019, at 17:33, Volker Simonis wrote: >>> >>> Hi Robin, >>> >>> thanks a lot for doing this! >>> >>> I have just a quick question. Do you know if any of the VSC indexers >>> (default, clangd) support call hierarchies (i.e. "called by", >>> "callers" of a function/method) and "used by" for variables/class >>> fields? >> >> Sure, I can make a quick summary of the various pros and cons of the >> indexers that I’ve found so far. They are all moving pretty fast though, so >> didn’t think it was a good fit for the documentation file. >> >> In general, VS Code itself only just recently gained proper support for >> displaying call hierarchies: >> https://urldefense.proofpoint.com/v2/url?u=https-3A__code.visualstudio.com_updates_v1-5F33-23-5Fcall-2Dhierarchy=DwIFaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=UJlIwvo0Thp_BYS_NWFT0ryBTIkcL2KhsFz8CKsa4GY=82pYT4_vb0KoCg2J7f8ZHebXioM2IbSDUpVgSuUAz-A=yY4u7WPyw8IMAQOytIDpU3MePzQMuGgCTyIP4XuHXbI= >> - but alternative indexers have worked around this by showing them >> differently. >> >> Default (Microsoft - C/C++ for Visual Studio Code) >> + Easy to setup, as no additional dependencies are needed >> + Good “go to definition”, the only one that can make sense of the >> template+macro stuff in log.hpp for example >> - Find references (used by) not done yet (said to be coming in the June >> update: >> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_microsoft_vscode-2Dcpptools_issues_15=DwIFaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=UJlIwvo0Thp_BYS_NWFT0ryBTIkcL2KhsFz8CKsa4GY=82pYT4_vb0KoCg2J7f8ZHebXioM2IbSDUpVgSuUAz-A=usQi8Sluzbg7sm51fLwBgFVSkY8uj_ZkOgFhT6clIjw=) >> - Call hierarchies also not there (scheduled for September apparently: >> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_microsoft_vscode-2Dcpptools_issues_16=DwIFaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=UJlIwvo0Thp_BYS_NWFT0ryBTIkcL2KhsFz8CKsa4GY=82pYT4_vb0KoCg2J7f8ZHebXioM2IbSDUpVgSuUAz-A=0nJoexh1jJuqrNQ5bhxtpwdzS9LhGYhzKyEmFbM1D8A=) >> >> clangd >> + Actively developed and part of the llvm/clang project >> + Support for find references >> - ..but not call hierarchies yet >> - Full project indexing is still flagged as experimental, and currently >> seems to fail when editing commonly used header files for example >> >> Full feature list: >> https://urldefense.proofpoint.com/v2/url?u=https-3A__clang.llvm.org_extra_clangd_Features.html-23complete-2Dlist-2Dof-2Dfeatures=DwIFaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=UJlIwvo0Thp_BYS_NWFT0ryBTIkcL2KhsFz8CKsa4GY=82pYT4_vb0KoCg2J7f8ZHebXioM2IbSDUpVgSuUAz-A=-59_vkfX4TV8qO7PoHopjLBC1c2Qu-HjneQnNM49Zps= >> >> rtags >> This is currently the most feature-complete indexer I think. But the VS Code >> integration is a bit minimal and not part of the rtags project itself, and >> it is missing things like indexer progress. >> + The indexer is actively developed and has been around for quite some time >> - You will probably have to build the indexer from source >> + Full support for call hierarchies and find references >> - VS Code integration a bit limited, missing convenience features like >> switch between header/source file, showing method/class documentation on >> hover. >> >> There are even more indexers of course, a promising one used to be “cquery", >> but that project seems to be defunct now. It lives on in the “ccls" indexer, >> but ithat one is a bit tricky to configure unless you use clang for building >> the JDK as well. >> >> So in summary, after the summer the default indexer might be the obvious >> best choice, but right now it depends on which features you think are the >> most important I guess.. >> > > Hi Robin, > > thanks a lot for the great summary! Incidentally I've just did some > Google research as well and basically came to the same conclusion. > "Cquery" seemed quite promising and also pretends to have call > hierarchies. But it seems to be defunct and I've also found some bug > reports about problems with the call hierarchies. Yeah, cquery did work okay when I tried it, but had some issues. However, the “spiritual” successor ccls has fixed all the issues I had with cquery, I spent a bit more time on configuring it today and finally figured out why it didn’t work! > For me "Called Hierarchy", and "Find References" (in this order) are > really the most important IDE features. I'm using Eclipse and if you > setup your HotSpot project correctly, these features work pretty well > and reliably. > > Have you personally tried the rtags "call hierarchies" / "find > references" support in VSC? From your summary it sounds like it's > worth giving it a try. Only tried it a bit, but I know other HotSpot developers that use it with emacs / vim. But the VS Code integration is really bare-bones, so a bit hard to use it
Re: RFR: 8223678: Add Visual Studio Code workspace generation support (for native code)
Hi Erik, > On 29 May 2019, at 17:22, Erik Joelsson wrote: > > Thanks, looks good! Thanks for reviewing! Unfortunately I had to do a few more general changes to get the “ccls" indexer working properly as well, hope you don’t mind taking another look: Webrevs: - Full: http://cr.openjdk.java.net/~rwestberg/8223678/webrev.03/ - Inc: http://cr.openjdk.java.net/~rwestberg/8223678/webrev.02-03/ Best regards, Robin > > /Erik > > On 2019-05-29 06:52, Robin Westberg wrote: >> Hi Erik, >> >> Thanks for taking a look! >> >>> On 28 May 2019, at 15:52, Erik Joelsson wrote: >>> >>> Hello Robin, >>> >>> Looks good. >>> >>> Adding of ide.md is very nice, but could you try to format it like >>> testing.md and building.md in regards to line lengths? I believe we are >>> trying for 80 chars in those to make them read reasonably in a standard >>> editor. >> Sure, did a bit of reflowing and updated some formatting in order to look >> more like the other .md files. >> >> Webrevs: >> - Full: https://cr.openjdk.java.net/~rwestberg/8223678/webrev.02/ >> - Inc: https://cr.openjdk.java.net/~rwestberg/8223678/webrev.01-02/ >> >> Best regards, >> Robin >> >>> /Erik >>> >>> On 2019-05-27 09:03, Robin Westberg wrote: Hi all, Please review this change that adds build system support for generating a Visual Studio Code workspace configured for working with the JDK native code. It configures the default C/C++ IntelliSense Engine to allow code completion/navigation and similar features. It also configures two executable targets (gtestLauncher and java) that can be built and debugged from the IDE. The main target is "make vscode-project”, additional information can be found in doc/ide.[md|html]. Issue: https://bugs.openjdk.java.net/browse/JDK-8223678 Webrev: https://cr.openjdk.java.net/~rwestberg/8223678/webrev.01/ Testing: Manual testing on Linux, MacOS and Windows Thanks Erik Joelsson for taking a look at an earlier version of it! Best regards, Robin
Re: RFR (M) 8225048: Shenandoah x86_32 support
On 5/30/19 3:29 PM, Erik Joelsson wrote: > Build change looks good. Thanks, Erik. Not expecting any more build changes, I am going to drop build-dev@ from the following replies on this thread. -Aleksey
RE: RFR 8193255: Root Certificates should be stored in text format and assembled at build time
Hi Max, this looks all good to me now :) Best regards Christoph > -Original Message- > From: build-dev On Behalf Of > Weijun Wang > Sent: Freitag, 31. Mai 2019 05:01 > To: Erik Joelsson > Cc: security-...@openjdk.java.net; build-dev d...@openjdk.java.net> > Subject: Re: RFR 8193255: Root Certificates should be stored in text format > and assembled at build time > > New webrev at > >https://cr.openjdk.java.net/~weijun/8193255/webrev.01 > > Changes: > > 1. Textual info at the beginning of each cert > > 2. Makefile: indentation, BUILD_TOOLS_JDK, MakeTargetDir, files instead of > dir > > Thanks, > Max > > > On May 31, 2019, at 1:34 AM, Erik Joelsson > wrote: > > > > On 2019-05-30 08:32, Weijun Wang wrote: > >> > >>> On May 30, 2019, at 10:01 PM, Erik Joelsson > wrote: > >>> > >>> In my experience, using directories for dependencies in make does not > work well. Since all the files in make/data/cacerts are in a flat structure, I > would recommend expressing the prerequisites as: > >>> > >>> $(wildcard $(GENDATA_CACERTS_SRC)/*) > >>> > >>> This will not cover the case where a file is removed, but that case is > rarely handled well in make based build systems. > >> But in my experiment, using the directory name does detect the file > removal. > > > > It believe that worked well on your machine, but directory timestamp > updates are file system dependent. I'm not sure we can count on all > filesystems to accurately reflect time stamps based on file modification. It's > also possible that an OS would touch directory timestamps for other reasons, > which should not affect the build. I haven't tried having source directories > as > prerequisites before, so I simply don't know how reliable it is. My experience > is rather with directories as targets, which certainly doesn't work. If you > verified that it worked as expected on all supported OSes, I would be less > worried. > > > >> Or, can I list *both* the files and the directory to get maximum > awareness? > > > > The directory modification time will usually not change when a file in it is > modified, only when adding or removing files (and possibly some other > operations), so adding the files is certainly a must. If you go with both, > then I > would just be worried about potential false positives. > > > > /Erik > >
