Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27500004:
Listen for new system certificates. (Closed)
|
Created:
7 years, 2 months ago by qsr Modified:
7 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, android-webview-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionListen for new system certificates.
Android use the system certificate database. This CL ensure that a
modification of the system certificates is received by services that
listen for those.
BUG=149426
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230126
Patch Set 1 #Patch Set 2 : Re-upload #
Total comments: 17
Patch Set 3 : Follow review. #Patch Set 4 : Follow review. #
Total comments: 10
Patch Set 5 : Follow review #
Total comments: 13
Patch Set 6 : Follow review #Patch Set 7 : Fix tests #Patch Set 8 : Fix comment #
Messages
Total messages: 27 (0 generated)
Hi Wan-Teh, Can you take a look at this change. I have been obliged to add a method on the Observer of CertDatabase. because in android we use the system database, and I do not have details about changes. It means I need a very broad notification for changes.
Patch set 2 LGTM. qsr: I added rsleevi as a second reviewer. Please wait for his approval. He knows the CertDatabase class better than I do. I remember a ChromeOS developer also wanted to make a change to the CertDatabase::Observer interface recently. https://codereview.chromium.org/27500004/diff/2001/net/android/java/src/org/c... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/27500004/diff/2001/net/android/java/src/org/c... net/android/java/src/org/chromium/net/X509Util.java:77: * BroadcastReceiver that listen to change in the system keystore to invalidate certificate Nit: listen => listens https://codereview.chromium.org/27500004/diff/2001/net/android/java/src/org/c... net/android/java/src/org/chromium/net/X509Util.java:151: * After each modification by the systen of the key store, trust manager has to be regenerated. Typo: systen => system https://codereview.chromium.org/27500004/diff/2001/net/android/java/src/org/c... net/android/java/src/org/chromium/net/X509Util.java:285: * Notify the native cert database that the system database has been updated. Nit: cert database => net::CertDatabase class to clarify that you are not referring to a database file. https://codereview.chromium.org/27500004/diff/2001/net/android/x509_util.cc File net/android/x509_util.cc (right): https://codereview.chromium.org/27500004/diff/2001/net/android/x509_util.cc#n... net/android/x509_util.cc:12: void NotifyCertDatabaseUpdated(JNIEnv* env, jclass clazz) { This method isn't declared in net/android/x509_util.h. Why is it defined in this file? https://codereview.chromium.org/27500004/diff/2001/net/cert/cert_database.h File net/cert/cert_database.h (right): https://codereview.chromium.org/27500004/diff/2001/net/cert/cert_database.h#n... net/cert/cert_database.h:47: virtual void OnDatabaseUpdated() {} Nit: a non specific manner => an unspecified manner? I suggest naming this method "OnDatabaseChanged" and the notification method "NotifyObserversOfDatabaseChanged". https://codereview.chromium.org/27500004/diff/2001/net/cert/cert_database.h#n... net/cert/cert_database.h:87: void NotifyObserversOfDatabaseUpdated(); Can this method be declared private, along with the other NotifyObserversOfXXX methods on lines 96-99? https://codereview.chromium.org/27500004/diff/2001/net/socket/client_socket_p... File net/socket/client_socket_pool_manager_impl.cc (right): https://codereview.chromium.org/27500004/diff/2001/net/socket/client_socket_p... net/socket/client_socket_pool_manager_impl.cc:403: FlushSocketPoolsWithError(ERR_NETWORK_CHANGED); We probably should just have OnCertAdded and OnCertTrustChanged call OnDatabaseUpdated. It will require updating this comment to make it applicable to all three observer methods.
https://codereview.chromium.org/27500004/diff/2001/net/cert/cert_database.h File net/cert/cert_database.h (right): https://codereview.chromium.org/27500004/diff/2001/net/cert/cert_database.h#n... net/cert/cert_database.h:47: virtual void OnDatabaseUpdated() {} Please mention why this broad notification is necessary on some platforms, and you can name Android as an example. Thanks.
https://codereview.chromium.org/27500004/diff/2001/net/android/java/src/org/c... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/27500004/diff/2001/net/android/java/src/org/c... net/android/java/src/org/chromium/net/X509Util.java:77: * BroadcastReceiver that listen to change in the system keystore to invalidate certificate On 2013/10/16 15:37:34, wtc wrote: > > Nit: listen => listens Done. https://codereview.chromium.org/27500004/diff/2001/net/android/java/src/org/c... net/android/java/src/org/chromium/net/X509Util.java:151: * After each modification by the systen of the key store, trust manager has to be regenerated. On 2013/10/16 15:37:34, wtc wrote: > > Typo: systen => system Done. https://codereview.chromium.org/27500004/diff/2001/net/android/java/src/org/c... net/android/java/src/org/chromium/net/X509Util.java:285: * Notify the native cert database that the system database has been updated. On 2013/10/16 15:37:34, wtc wrote: > > Nit: cert database => net::CertDatabase class > > to clarify that you are not referring to a database file. Done. https://codereview.chromium.org/27500004/diff/2001/net/android/x509_util.cc File net/android/x509_util.cc (right): https://codereview.chromium.org/27500004/diff/2001/net/android/x509_util.cc#n... net/android/x509_util.cc:12: void NotifyCertDatabaseUpdated(JNIEnv* env, jclass clazz) { On 2013/10/16 15:37:34, wtc wrote: > > This method isn't declared in net/android/x509_util.h. Why is it defined in this > file? It is defined in jni/X509Util_jni.h The signature and plumbing for this method is autogenerated by the android build, as this is the method that is called when the nativeNotifyCertDatabaseUpdated method is called on the X509Util java class. https://codereview.chromium.org/27500004/diff/2001/net/cert/cert_database.h File net/cert/cert_database.h (right): https://codereview.chromium.org/27500004/diff/2001/net/cert/cert_database.h#n... net/cert/cert_database.h:47: virtual void OnDatabaseUpdated() {} On 2013/10/16 15:37:34, wtc wrote: > > Nit: a non specific manner => an unspecified manner? > > I suggest naming this method "OnDatabaseChanged" and the notification method > "NotifyObserversOfDatabaseChanged". Done. https://codereview.chromium.org/27500004/diff/2001/net/cert/cert_database.h#n... net/cert/cert_database.h:87: void NotifyObserversOfDatabaseUpdated(); On 2013/10/16 15:37:34, wtc wrote: > > Can this method be declared private, along with the other NotifyObserversOfXXX > methods on lines 96-99? Not really. It needs to be called externally by the java class that gets notified when the system database change. Do you prefer a level of indirection? That would still mean a public method that would just call the private one. https://codereview.chromium.org/27500004/diff/2001/net/socket/client_socket_p... File net/socket/client_socket_pool_manager_impl.cc (right): https://codereview.chromium.org/27500004/diff/2001/net/socket/client_socket_p... net/socket/client_socket_pool_manager_impl.cc:403: FlushSocketPoolsWithError(ERR_NETWORK_CHANGED); I did not do this because I'm not sure to understand the comments well enough. The comments says that we should flush when we remove trust, but not when we add, *but*, the code is flushing on |OnCertAdded| and not on |OnCertRemoved|. There is a subtlety I'm missing here.
Adding joth@ for webview digit@ for net/android
Hi qsr, thanks for taking this on. I apologize that this is a less than ideal API as it stands today, but I'm a little concerned with the proposal. I have a suggestion & explanation below that hopefully tries to clarify how things work. Since this is a common interface across platforms, I'd like to try and keep the behaviour defined for each method, otherwise we get into further confusing territory. This is admittedly a bit hard, since every platform has different capabilities and a different concept of trust stores, but hopefully the replies below show where the commonality is. Note that the existing behaviour is to call OnCertTrustChanged(NULL) if it's not known what certificate was changed. Cheers, Ryan https://codereview.chromium.org/27500004/diff/2001/net/socket/client_socket_p... File net/socket/client_socket_pool_manager_impl.cc (right): https://codereview.chromium.org/27500004/diff/2001/net/socket/client_socket_p... net/socket/client_socket_pool_manager_impl.cc:403: FlushSocketPoolsWithError(ERR_NETWORK_CHANGED); On 2013/10/16 16:02:09, qsr wrote: > I did not do this because I'm not sure to understand the comments well enough. > > The comments says that we should flush when we remove trust, but not when we > add, *but*, the code is flushing on |OnCertAdded| and not on |OnCertRemoved|. > There is a subtlety I'm missing here. OnCertAdded == OnClientCertAdded. The flushing on adding a client cert is strongly undesirable, an artifact of when client cert detection was in the SSLClientSocket, as this was needed to flush caches. To put a 'proper' solution on the table: OnCertAdded -> renamed to OnClientCertAdded Have the ClientCertStore listening for OnClientCertadded. When a new cert is added, it: [short term] only flushes the socket pools if there was a client cert request that would change [long term] only flushes the sockets for the client cert that changed OnCertTrustChanged is a bit of a misnomer as well; it's likely we should just call this OnDatabaseUpdated, and try to specifically enumerate the cases that we care about (since some systems will raise additional alerts we don't care about) https://codereview.chromium.org/27500004/diff/12001/net/cert/cert_database.h File net/cert/cert_database.h (right): https://codereview.chromium.org/27500004/diff/12001/net/cert/cert_database.h#... net/cert/cert_database.h:50: virtual void OnDatabaseChanged() {} I would like to avoid adding a new method, especially to a base interface, when it's a platform-specific interface. For example, how could Windows implement this method, and what would it mean?
https://codereview.chromium.org/27500004/diff/12001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/27500004/diff/12001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1315: return SslUtil.getCertificateFromDerBytes(mContainerView.getContext(), I think this should be application context, i.e. mContentViewCore.getContext().getApplicationContext() ? https://codereview.chromium.org/27500004/diff/12001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/SslUtil.java (right): https://codereview.chromium.org/27500004/diff/12001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslUtil.java:63: } revert this change. looks stale to me. https://codereview.chromium.org/27500004/diff/12001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/27500004/diff/12001/net/android/java/src/org/... net/android/java/src/org/chromium/net/X509Util.java:46: Log.e(TAG, "Uable to reload the default TrustManager", s/Uable/Unable/ (fix the log messages below too.)
Review comments on patch set 4: https://codereview.chromium.org/27500004/diff/2001/net/cert/cert_database.h File net/cert/cert_database.h (right): https://codereview.chromium.org/27500004/diff/2001/net/cert/cert_database.h#n... net/cert/cert_database.h:87: void NotifyObserversOfDatabaseUpdated(); On 2013/10/16 16:02:09, qsr wrote: > It needs to be called externally by the java class that gets > notified when the system database change. Do you prefer a level of indirection? Not when I asked you that question. As a code reviewer, I was puzzled why the existing NotifyObserversOfXXX methods can be private, but NotifyObserversOfDatabaseUpdated can't. Perhaps you can name the public method something like "OnAndroidKeyChainChanged", and have it call NotifyObserversOfCertTrustChanged(NULL). https://codereview.chromium.org/27500004/diff/12001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/27500004/diff/12001/net/android/java/src/org/... net/android/java/src/org/chromium/net/X509Util.java:41: if (intent.getAction().equals(KeyChain.ACTION_STORAGE_CHANGED)) { Based on the documentation of ACTION_STORAGE_CHANGED at http://developer.android.com/reference/android/security/KeyChain.html#ACTION_..., I agree with rsleevi that this is most similar to OnCertTrustChanged(NULL).
https://codereview.chromium.org/27500004/diff/12001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/27500004/diff/12001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1315: return SslUtil.getCertificateFromDerBytes(mContainerView.getContext(), On 2013/10/16 18:19:39, sgurun wrote: > I think this should be application context, i.e. > mContentViewCore.getContext().getApplicationContext() ? Done. Thanks. https://codereview.chromium.org/27500004/diff/12001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/SslUtil.java (right): https://codereview.chromium.org/27500004/diff/12001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/SslUtil.java:63: } On 2013/10/16 18:19:39, sgurun wrote: > revert this change. looks stale to me. Will be hard. The file was missing a end of line newline. Should not happen... https://codereview.chromium.org/27500004/diff/12001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/27500004/diff/12001/net/android/java/src/org/... net/android/java/src/org/chromium/net/X509Util.java:41: if (intent.getAction().equals(KeyChain.ACTION_STORAGE_CHANGED)) { On 2013/10/16 18:33:24, wtc wrote: > > Based on the documentation of ACTION_STORAGE_CHANGED at > http://developer.android.com/reference/android/security/KeyChain.html#ACTION_..., > I agree with rsleevi that this is most similar to OnCertTrustChanged(NULL). Done. https://codereview.chromium.org/27500004/diff/12001/net/android/java/src/org/... net/android/java/src/org/chromium/net/X509Util.java:46: Log.e(TAG, "Uable to reload the default TrustManager", On 2013/10/16 18:19:39, sgurun wrote: > s/Uable/Unable/ (fix the log messages below too.) Done. https://codereview.chromium.org/27500004/diff/12001/net/cert/cert_database.h File net/cert/cert_database.h (right): https://codereview.chromium.org/27500004/diff/12001/net/cert/cert_database.h#... net/cert/cert_database.h:50: virtual void OnDatabaseChanged() {} On 2013/10/16 17:40:52, Ryan Sleevi wrote: > I would like to avoid adding a new method, especially to a base interface, when > it's a platform-specific interface. > > For example, how could Windows implement this method, and what would it mean? Not sure to understand the comment, as the observer has nothing platform specific. What is plaform specific is whether this is ever called, so user should not care. Now, given that OnCertTrustChanged(NULL) is in fact doing the same thing (but this behavior is not documented), I agree it is not useful. Removed.
Patch set 5 LGTM. https://codereview.chromium.org/27500004/diff/52001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/27500004/diff/52001/net/android/java/src/org/... net/android/java/src/org/chromium/net/X509Util.java:287: private static native void nativeNotifyCertDatabaseUpdated(); Nit: Perhaps this native method should be renamed "nativeNotifyKeyChainChanged" or "NotifyAndroidKeyChainChanged"? https://codereview.chromium.org/27500004/diff/52001/net/cert/cert_database_an... File net/cert/cert_database_android.cc (right): https://codereview.chromium.org/27500004/diff/52001/net/cert/cert_database_an... net/cert/cert_database_android.cc:42: make_scoped_refptr(cert)); Can you just pass NULL as the second argument?
lgtm https://codereview.chromium.org/27500004/diff/52001/net/cert/cert_database_an... File net/cert/cert_database_android.cc (right): https://codereview.chromium.org/27500004/diff/52001/net/cert/cert_database_an... net/cert/cert_database_android.cc:42: make_scoped_refptr(cert)); On 2013/10/17 20:43:59, wtc wrote: > > Can you just pass NULL as the second argument? scoped_ptr<X509Certificate>() is the preferred syntax
On 2013/10/17 22:07:51, Ryan Sleevi wrote: > lgtm > > https://codereview.chromium.org/27500004/diff/52001/net/cert/cert_database_an... > File net/cert/cert_database_android.cc (right): > > https://codereview.chromium.org/27500004/diff/52001/net/cert/cert_database_an... > net/cert/cert_database_android.cc:42: make_scoped_refptr(cert)); > On 2013/10/17 20:43:59, wtc wrote: > > > > Can you just pass NULL as the second argument? > > scoped_ptr<X509Certificate>() is the preferred syntax lgtm android_webview joth is the owner though.
https://codereview.chromium.org/27500004/diff/52001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/27500004/diff/52001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1316: mContentViewCore.getContext().getApplicationContext(), doesn't make much odds, but mContainerView.getContext().getApplicationContext() would be more conventional. (but seem my comments elsewhere asking if this is needed at all) https://codereview.chromium.org/27500004/diff/52001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/27500004/diff/52001/net/android/java/src/org/... net/android/java/src/org/chromium/net/X509Util.java:47: e); nit: use 100-columns in java, no need to line wrap. x3 (when you do line wrap, indent by 8 chars, don't align with open paren) https://codereview.chromium.org/27500004/diff/52001/net/android/java/src/org/... net/android/java/src/org/chromium/net/X509Util.java:117: context.registerReceiver(sTrustStorageListener, rather than plumb the context through numerous layers to get here, is there a reason not to use base::android::GetApplicationContext() ? (After all, majority of callers are using that to seed the context they pass all the way through here anyway) you'll need to add an additional 'native' method in this class to do that, but seems OK https://codereview.chromium.org/27500004/diff/52001/net/android/java/src/org/... net/android/java/src/org/chromium/net/X509Util.java:118: new IntentFilter(KeyChain.ACTION_STORAGE_CHANGED)); again, don't align with open paren
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/27500004/70001
https://chromiumcodereview.appspot.com/27500004/diff/52001/android_webview/ja... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://chromiumcodereview.appspot.com/27500004/diff/52001/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContents.java:1316: mContentViewCore.getContext().getApplicationContext(), N/A anymore. https://chromiumcodereview.appspot.com/27500004/diff/52001/net/android/java/s... File net/android/java/src/org/chromium/net/X509Util.java (right): https://chromiumcodereview.appspot.com/27500004/diff/52001/net/android/java/s... net/android/java/src/org/chromium/net/X509Util.java:47: e); On 2013/10/17 22:43:54, joth wrote: > nit: use 100-columns in java, no need to line wrap. x3 > > (when you do line wrap, indent by 8 chars, don't align with open paren) Done. https://chromiumcodereview.appspot.com/27500004/diff/52001/net/android/java/s... net/android/java/src/org/chromium/net/X509Util.java:117: context.registerReceiver(sTrustStorageListener, On 2013/10/17 22:43:54, joth wrote: > rather than plumb the context through numerous layers to get here, is there a > reason not to use base::android::GetApplicationContext() ? (After all, majority > of callers are using that to seed the context they pass all the way through here > anyway) > > you'll need to add an additional 'native' method in this class to do that, but > seems OK Done. > https://chromiumcodereview.appspot.com/27500004/diff/52001/net/android/java/s... net/android/java/src/org/chromium/net/X509Util.java:118: new IntentFilter(KeyChain.ACTION_STORAGE_CHANGED)); On 2013/10/17 22:43:54, joth wrote: > again, don't align with open paren Done. https://chromiumcodereview.appspot.com/27500004/diff/52001/net/android/java/s... net/android/java/src/org/chromium/net/X509Util.java:287: private static native void nativeNotifyCertDatabaseUpdated(); On 2013/10/17 20:43:59, wtc wrote: > > Nit: Perhaps this native method should be renamed "nativeNotifyKeyChainChanged" > or "NotifyAndroidKeyChainChanged"? Done. https://chromiumcodereview.appspot.com/27500004/diff/52001/net/cert/cert_data... File net/cert/cert_database_android.cc (right): https://chromiumcodereview.appspot.com/27500004/diff/52001/net/cert/cert_data... net/cert/cert_database_android.cc:42: make_scoped_refptr(cert)); On 2013/10/17 22:07:51, Ryan Sleevi wrote: > On 2013/10/17 20:43:59, wtc wrote: > > > > Can you just pass NULL as the second argument? > > scoped_ptr<X509Certificate>() is the preferred syntax Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/27500004/70001
Retried try job too often on win7_aura for step(s) app_list_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/27500004/70001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/27500004/70001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/27500004/810001
Message was sent while issue was closed.
Change committed as 230126
Message was sent while issue was closed.
On 2013/10/22 14:35:17, I haz the power (commit-bot) wrote: > Change committed as 230126 This has broken running gyp with --check because it introduces two files with the same basename in the same target, which is disallowed because MSVC doesn't support it (there are now two x509_util.cc's). Unfortunately few/none of the bots run gyp with --check currently.
Message was sent while issue was closed.
On 2013/10/22 17:17:30, Torne wrote:
>
> This has broken running gyp with --check because it introduces two files with
> the same basename in the same target, which is disallowed because MSVC doesn't
> support it (there are now two x509_util.cc's).
We can fix this by renaming those files x509_util_android.{h,cc}.
Torne, can this be fixed by excluding the files for OS=="win" using 'sources!'
or 'sources/'?
Message was sent while issue was closed.
On 2013/10/22 21:22:59, wtc wrote:
> On 2013/10/22 17:17:30, Torne wrote:
> >
> > This has broken running gyp with --check because it introduces two files
with
> > the same basename in the same target, which is disallowed because MSVC
doesn't
> > support it (there are now two x509_util.cc's).
>
> We can fix this by renaming those files x509_util_android.{h,cc}.
>
> Torne, can this be fixed by excluding the files for OS=="win" using 'sources!'
> or 'sources/'?
Torne already landed a fix for this.
Message was sent while issue was closed.
On 2013/10/22 21:25:22, Ryan Sleevi wrote:
> On 2013/10/22 21:22:59, wtc wrote:
> > On 2013/10/22 17:17:30, Torne wrote:
> > >
> > > This has broken running gyp with --check because it introduces two files
> with
> > > the same basename in the same target, which is disallowed because MSVC
> doesn't
> > > support it (there are now two x509_util.cc's).
> >
> > We can fix this by renaming those files x509_util_android.{h,cc}.
> >
> > Torne, can this be fixed by excluding the files for OS=="win" using
'sources!'
> > or 'sources/'?
>
> Torne already landed a fix for this.
er, Nyquist did
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
