search

trendmicro / tlsh

Other
726 stars 135 forks source link

Pull request #66 #134

Closed HydraDragonAntivirus closed 5 months ago

HydraDragonAntivirus commented 6 months ago

Just a few fixes. I converted #if defined WINDOWS || defined MINGW to #if defined(WIN32) || defined(_WIN32) || defined(__WIN32) && !defined(CYGWIN)

HydraDragonAntivirus commented 6 months ago

If you want I can add x64 version of tlsh.dll or tlsh.lib codes. I already change project for this.

HydraDragonAntivirus commented 6 months ago

I don't believe there any issues on C++ at TLSH code. So please close issues related C++ and accept my ideas.

jonjoliver commented 5 months ago

Hi HydraDragonAntivirus

I have reviewed your merge request I see a problem - this will break the MINGW compile instructions (this version was requested a few years back by VirusTotal) See README.mingw

Could you change you pull request to be the following changes in include/tlsh.h include/tlsh_impl.h

if defined WINDOWS || defined MINGW =>

if defined(WIN32) || defined(_WIN32) || defined(__WIN32) || defined(WINDOWS) || defined(MINGW)

test/tlsh_unittest.cpp

ifdef WINDOWS

=>

if defined(WIN32) || defined(_WIN32) || defined(__WIN32) || defined(WINDOWS)

We then want to carefully add the "&& !defined(CYGWIN)" part to do the operator precendence correctly

jonjoliver commented 5 months ago

Also HydraDragonAntiVirus I see that github says "Merging is blocked. Merging can be performed automatically with 2 approving reviews." I might have trouble merging. Lets see how it goes. I might have to start a new tlsh repo where I keep it maintained (I will do that if I can't get merges approved)

HydraDragonAntivirus commented 5 months ago

Are you here?

jonjoliver commented 5 months ago

Sorry - just crazy busy. Just doing best efforts from outside TM...

I have reviewed - I will approve right now and try to get this merged into master

jonjoliver commented 5 months ago

OK I need another reviewer to approve. Let me find one

jonjoliver commented 5 months ago

Hi HydraDragonAntivirus

I contacted JaysonPryde to help - but apparently the 2 of us do not have permission to merge a branch. I am going to have to find someone else from Trend...

jonjoliver commented 5 months ago

Thanks Fernando (@merces)

jonjoliver commented 5 months ago

@HydraDragonAntivirus - I recruited some help from Merces and JaysonPryde (thank you). I am merging. Please test.