Quantcast

[GitHub] commons-lang pull request #231: Evaluate Architecure

classic Classic list List threaded Threaded
71 messages Options
1234
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang pull request #231: Evaluate Architecure

coveralls
GitHub user Tomschi opened a pull request:

    https://github.com/apache/commons-lang/pull/231

    Evaluate Architecure

    Add static field for hardware architecure.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/Tomschi/commons-lang master

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/commons-lang/pull/231.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #231
   
----
commit 13df6b084b0ac64e291d10f7ecdb4dc85e7cde91
Author: Tomschi <[hidden email]>
Date:   2017-02-02T20:29:39Z

    Add boolean variables for evaluating architecture.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
Github user coveralls commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
   
    [![Coverage Status](https://coveralls.io/builds/9969109/badge)](https://coveralls.io/builds/9969109)
   
    Coverage increased (+0.0007%) to 94.456% when pulling **13df6b084b0ac64e291d10f7ecdb4dc85e7cde91 on Tomschi:master** into **ab25f67348d4885620df86497889c1826f013a73 on apache:master**.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user kinow commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    I remember a discussion about it some time ago.
   
    The issue with this approach was that os.arch tells only the JVM arch, not really OS arch.
   
    If there is a strong use case for these properties, we would have to be careful with the javadocs. Right now the javadocs don't explicitly tell the user that it is the JVM arch, and not the OS arch.
   
    Other approaches involve reflection or JNI, to find out the OS arch, but I think that is a bit too tricky to get it done correctly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user PascalSchumacher commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    The discussion mentioned by @kinow is here: https://issues.apache.org/jira/browse/LANG-1145


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user coveralls commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
   
    [![Coverage Status](https://coveralls.io/builds/10120247/badge)](https://coveralls.io/builds/10120247)
   
    Coverage decreased (-0.006%) to 94.53% when pulling **a5945e7b0b722aab1693d3912b76c0042fc74cee on Tomschi:master** into **b715d18f09591c510dded3a447a3ad1aacfa2173 on apache:master**.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user Tomschi commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    Rewrite javadoc's.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user kinow commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    Thanks for updating the pull request @Tomschi.
   
    I don't have a use case for this. I can see where it could be used, but I don't have any project where I would use it. Code is clear, javadocs have been updated :-)
   
    So will leave it here for others to review, comment, and maybe perhaps.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user Tomschi commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    I'am using it for the jacob-project. There i have to know, if it is a 32 or 64 bit architecture to load the correct dll library.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user kinow commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    Gotcha, found this example https://github.com/Tomschi/jacob-parent/blob/ec3f1c10169c26f14ee1f61bd6622c67a73e26fc/jacob/src/main/java/com/jacob/com/LibraryLoader.java#L202
   
    Looks like a valid use case. I'm all right with merging it, my +0. Let's now wait to see what others think about it.
   
    Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user PascalSchumacher commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    I think this is a worthy addition.
   
    In my experience people often do not read documentation. Maybe we should use `IS_32_BIT_JVM` so there can be no confusion? Or is this is too paranoid? What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user kinow commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    Oh, good point @PascalSchumacher have no objection to it. We could probably avoid a few misunderstandings that way. Happy with that too @Tomschi ?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user Tomschi commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    It's ok for me, i will change it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user coveralls commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
   
    [![Coverage Status](https://coveralls.io/builds/10210615/badge)](https://coveralls.io/builds/10210615)
   
    Coverage increased (+0.001%) to 94.53% when pulling **3e335fd2046ad3cf6bd84456b836d8025998c321 on Tomschi:master** into **30c85ad05363767deeefee577063c2c432b971d4 on apache:master**.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user PascalSchumacher commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    jira issue: https://issues.apache.org/jira/browse/LANG-1313
   
    I plan to merge this tomorrow (if there are no objections).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user michael-o commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    What is the real pupose for this actually? The client should not care about the arch at all. The regex match is brittle. This will likely fail on ARM and it fails here on Itanium with HP-UX for `os.arch` `IA64N` which is a 32 bit VM.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user kinow commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    @michael-o
   
    >What is the real pupose for this actually? The client should not care about the arch at all.
   
    I think @Tomschi use case is valid, where a client could need to know the arch before loading a certain library, and we had another issue submitted LANG-1145 with similar requirement.
   
    >The regex match is brittle. This will likely fail on ARM and it fails here on Itanium with HP-UX for os.arch IA64N which is a 32 bit VM.
   
    Note taken, perhaps before merging we can try to cover more archs, like this list:
   
    * http://lopica.sourceforge.net/os.html
   
    There is another place where arch is used within Commons too:
   
    * https://github.com/apache/commons-crypto/blob/158be0644c353a617427ab190a4f09082cda42ac/src/main/java/org/apache/commons/crypto/OsInfo.java#L28
   
    We can possibly look at how crypto is using it, and if we could maybe use a similar approach here.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user michael-o commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    @kinow The cheapest way is to produce two bundles, one for 32 bit and one for 64 bit, if possible. The lopica source is useless, it is 15 years old. Commons Crypto is better. hawtjni on GitHub has decent detection code. It is work checking. But the current approach is way too simple.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user kinow commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    Looks better now @Tomschi did you use Crypto's code as reference? cc @michael-o


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user Tomschi commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    Yes, the base of my code references to the commons crypto lib.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-lang issue #231: Evaluate Architecure

coveralls
In reply to this post by coveralls
Github user kinow commented on the issue:

    https://github.com/apache/commons-lang/pull/231
 
    Excellent @Tomschi
   
    I'm dropping a note to the mailing list to ask for feedback from crypto devs, as there could be some synergy.
   
    I will play with the code at home, but one thing that I noticed is that it seems to contain tabs... minor nit pick, but could you check checkstyle and make sure the code is not introducing any warnings / errors there, please? That way it will be easier to merge the PR.
   
    Thanks!
    B


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
1234
Loading...