New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enabled arclink decryption during download #1360

Merged
merged 6 commits into from Mar 5, 2017

Conversation

Projects
None yet
3 participants
@barsch
Member

barsch commented Mar 29, 2016

lesser memory footprint while having the same execution speed as before

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Mar 29, 2016

Member

coverage/coveralls lies - it includes files which were not changed in this PR - however I really love the idea of this feature if it would work ...

Member

barsch commented Mar 29, 2016

coverage/coveralls lies - it includes files which were not changed in this PR - however I really love the idea of this feature if it would work ...

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 29, 2016

Member

Yea coveralls is funny sometimes...no clue why it lists untouched files.

The PR itself looks fine to me. I assume you locally tested on py2 and py3?

Member

krischer commented Mar 29, 2016

Yea coveralls is funny sometimes...no clue why it lists untouched files.

The PR itself looks fine to me. I assume you locally tested on py2 and py3?

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Mar 29, 2016

Member

tests were fine for windows - still would be nice if someone would test it manually on linux

Member

barsch commented Mar 29, 2016

tests were fine for windows - still would be nice if someone would test it manually on linux

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 29, 2016

Member

I get lots of timeouts when running test_decrypt.py but can run the normal arclinks tests just fine. Is this expected? Is encrypted data send over a different port?

Member

krischer commented Mar 29, 2016

I get lots of timeouts when running test_decrypt.py but can run the normal arclinks tests just fine. Is this expected? Is encrypted data send over a different port?

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Mar 29, 2016

Member

hmm not good - I'll check at home if this is still running for me and report back - thanks for testing

Member

barsch commented Mar 29, 2016

hmm not good - I'll check at home if this is still running for me and report back - thanks for testing

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch
Member

barsch commented Mar 29, 2016

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 29, 2016

Member

I see. I'll test at home.

Member

krischer commented Mar 29, 2016

I see. I'll test at home.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 29, 2016

Member

FYI: I just pushed a commit to this branch that allows to run the crypto tests with either supported crypto library.

Member

krischer commented Mar 29, 2016

FYI: I just pushed a commit to this branch that allows to run the crypto tests with either supported crypto library.

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Mar 29, 2016

Member

careful: EVPError is only defined for M2Crypto ! -> unfortunatly as far I tested pycrypto does not raise anything if there is an error during decryption - so we have to stick with M2Crypto as default and only take pyCrypto as fallback

Member

barsch commented Mar 29, 2016

careful: EVPError is only defined for M2Crypto ! -> unfortunatly as far I tested pycrypto does not raise anything if there is an error during decryption - so we have to stick with M2Crypto as default and only take pyCrypto as fallback

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 29, 2016

Member

Hmm...there must be a way to figure out if the decrypt was successful, right? How does m2crypto even know? I mean we could always check the resulting miniseed file but m2crypto clearly does something else.

Member

krischer commented Mar 29, 2016

Hmm...there must be a way to figure out if the decrypt was successful, right? How does m2crypto even know? I mean we could always check the resulting miniseed file but m2crypto clearly does something else.

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Mar 29, 2016

Member

while using pycrypto it just fails later - either because it can't unpack using bzip2 or it can't read as its not a valid mseed

AFAIK m2crypto checks in the final call https://github.com/obspy/obspy/blob/master/obspy/clients/arclink/decrypt.py#L66 - so we could look into that

Member

barsch commented Mar 29, 2016

while using pycrypto it just fails later - either because it can't unpack using bzip2 or it can't read as its not a valid mseed

AFAIK m2crypto checks in the final call https://github.com/obspy/obspy/blob/master/obspy/clients/arclink/decrypt.py#L66 - so we could look into that

@megies megies added this to the 1.1.0 milestone Mar 31, 2016

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 5, 2016

Member

+TESTS:clients.arclink

Travis should run ArcLink test suites for future commits/pushes to this branch now (see here). Please ping me if it doesn't work..

Member

megies commented Oct 5, 2016

+TESTS:clients.arclink

Travis should run ArcLink test suites for future commits/pushes to this branch now (see here). Please ping me if it doesn't work..

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Nov 15, 2016

Member

What's the status here, @barsch @krischer?

Member

megies commented Nov 15, 2016

What's the status here, @barsch @krischer?

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Nov 16, 2016

Member

the later commit from @krischer does imho not work - see comment above - so either we stick with my orginal implementation, leave it as it is or find something better

and triggering a new test run did not work, or?

Member

barsch commented Nov 16, 2016

the later commit from @krischer does imho not work - see comment above - so either we stick with my orginal implementation, leave it as it is or find something better

and triggering a new test run did not work, or?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Nov 16, 2016

Member

and triggering a new test run did not work, or?

I rebased and force-pushed right now, that will trigger CI..

Member

megies commented Nov 16, 2016

and triggering a new test run did not work, or?

I rebased and force-pushed right now, that will trigger CI..

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Nov 16, 2016

Member

Current Travis commit status includes Arclink tests: https://travis-ci.org/obspy/obspy/builds/176344138

Not sure if tests cover decryption, though?

Member

megies commented Nov 16, 2016

Current Travis commit status includes Arclink tests: https://travis-ci.org/obspy/obspy/builds/176344138

Not sure if tests cover decryption, though?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Nov 17, 2016

Member

I'll bump this to 1.2.0. Feel free to add to 1.1.0 if it gets ready in the next couple of weeks.

Member

megies commented Nov 17, 2016

I'll bump this to 1.2.0. Feel free to add to 1.1.0 if it gets ready in the next couple of weeks.

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Mar 5, 2017

Member

imho ready to merge

Member

barsch commented Mar 5, 2017

imho ready to merge

enabled arclink decryption during download
lesser memory footprint (~50%) while having the same execution speed as
before

krischer and others added some commits Mar 29, 2016

@megies

megies approved these changes Mar 5, 2017 edited

Yep, CI looks good (we really should figure out why we have these erratic arclink "UNSET" problems.. or well.. once the VCR stuff is ready we're rid of it I guess)

Rebased on current master, mainly to get rid of some merge commits within this branch..

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Mar 5, 2017

Member

failing test is not related - merging

Member

barsch commented Mar 5, 2017

failing test is not related - merging

@barsch barsch merged commit 62ef604 into master Mar 5, 2017

4 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
docker-deb-buildbot Deb packaging succeeded but tests failed
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
docker-testbot Docker tests succeeded
Details

@barsch barsch deleted the arclink_decryption branch Mar 5, 2017

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 13, 2017

Member

@barsch this PR should also have modified arclink extra/optional dependencies.. anybody know how to modify a set of OR dependencies in setup.py? I couldn't find it..

https://github.com/obspy/obspy/blob/master/setup.py#L116

Should be semantically be something like..

 'arclink': ['m2crypto | pycrypto | cryptography'],
Member

megies commented Mar 13, 2017

@barsch this PR should also have modified arclink extra/optional dependencies.. anybody know how to modify a set of OR dependencies in setup.py? I couldn't find it..

https://github.com/obspy/obspy/blob/master/setup.py#L116

Should be semantically be something like..

 'arclink': ['m2crypto | pycrypto | cryptography'],
@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Mar 13, 2017

Member

I don't think there is such option - I would take it out completely and just document it

btw: pycryptodome does also work - its an drop-in replacement for pycrypto

Member

barsch commented Mar 13, 2017

I don't think there is such option - I would take it out completely and just document it

btw: pycryptodome does also work - its an drop-in replacement for pycrypto

megies added a commit that referenced this pull request Mar 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment