Allows for compilation on Linux #101

psi29a · 2017-01-12T08:49:14Z

This is a cross-platform fix that should allow for building on Linux (tested), Windows (untested) and MacOS (untested).

use fmin/fmax as they are provided for us in C++11, removed min/max
removed windows.h
added additional typedefs and defines
use a gpl version of dxgiformat
also replaced tabs with spaces because tabs are evil ;)

To compile on Debian and Ubuntu, I use:

/usr/lib/x86_64-linux-gnu/qt5/bin/qmake -makefile NifSkope.pro

… defines in addition to pulling in a gpl version of dxgiformat, also replaced tabs with spaces because tabs are evil ;) bring in dxgiformat

psi29a · 2017-01-12T08:50:40Z

Once this is merged, I would like to do more work:

add Travis-CI and Appveyor for some CI on Windows, Linux and MacOS
create an ITP for Debian (and by extension Ubuntu) so that we can get this packaged for downstream use.
additional building documentation

hexabits · 2017-01-31T16:49:34Z

also replaced tabs with spaces because tabs are evil ;)

I don't see where this was actually being done, but the NifSkope codebase is tabs-only, except block comments and for multi-line function parameter alignment, so it's a practice you'll need to adopt.

I unfortunately never got around to finishing this: https://github.com/niftools/nifskope/wiki/Coding-Style-Guide ... But it would include that new documents must use tabs and you must otherwise match the existing uses of spaces or tabs in existing documents. And the existing documents are all tabs.

MiroslavR · 2017-01-31T17:08:11Z

I don't see where this was actually being done

In NifSkope.pro.

hexabits · 2017-01-31T17:41:20Z

I am concerned about the use of fmin/fmax in a non-floating point context. None of the values being compared are actually floats and just because C++11 allows using integer types now doesn't mean that we should. I would feel better using std::min/std::max myself.

With that said using fmin/fmax might be faster by using intrinsics, and theoretically none of the integer inputs would exceed integer representation (+/- (2^24 + 1)) and thus wouldn't have precision issues.

psi29a · 2017-02-01T08:59:23Z

@jonwd7 thanks for reviewing, I reverted the formatting (tab2space) on NifSkope.pro. I also swapped out cmath's fmin/fmax for algorithm's std::min/max.

Does this help? :D

I have some travis-ci goodness in the pipe-line that requires these fixes to continue. ;)

BTW: your WIP code style guidelines mentions this:

Give empty bodies their own newlines for each bracket. For constructor initializers, keep the parent class on the same line as the definition, and any other members on the next line, indented 4 spaces.

I assumed that this is literally 4 spaces, not 1 tab. It would imply that any code written also be in spaces, not in tabs. I was using QtCreator's defaults on cleaning up before saving. I've disabled this for now.

We can do a reformatting in the future if this is wished. :)

add fsengine back in

try to compile now turn on sudo and remove qt4

formatting try without debian-sid

try getting qt5 from ppa try to get qt latest git

try to trim time down and add clang

try setting path for qt5 on osx build ;

go recursive build the damn thing try niftools#3 try to get a dir listing give me more output try vcxproj instead of sln try build dir try defaults fix bad yaml try building now

almost there try to get a nifskope dir case sensative try niftools#2 try niftools#3 get it right try build_name try hardcoded

psi29a · 2017-02-02T15:09:29Z

Just added appveyor and travis-ci goodness so that we now build on: Linux, MacOS and Windows (32/64/release/debug).

hexabits · 2017-02-03T03:19:27Z

Also, I'll leave it completely up to you to decide what to do with the files in install/linux-install and also makeconfig.sh in build. If whatever you are doing can work regardless of Linux distro, then we no longer need to keep those files. They were only there for posterity until a replacement was found.

Also, do the .yml files have to be in the project root? I assume for the two added so far the answer is yes, but I'm just bringing it up anyway. I think any additional files needed for any platform should go under build or install whenever possible.

Lastly, I saw that you've moved fsengine to all platforms. Historically I'm not sure of the reasons for making it Windows-only (maybe back in Qt4 when it used a virtual filesystem), but I'm guessing that the .pro file isn't the only place where this assumption is made. I wouldn't spend too much time getting it to work, as the entirety of the BSA loading code just needs to be completely replaced. It's an utter mess. Anyway, afaik there are several macro guards to keep certain functionality to Windows only at least for texture loading from archive, but I don't think that I did any such guards with my Archive Browser (mesh archives).

psi29a · 2017-02-03T09:09:51Z

Thanks @jonwd7 ,

I'll be honest, I'm biased toward Debian/Ubuntu/and derivatives because that is what I work with day-in/day-out. That being said, what I'm doing isn't linux-distro dependent. The only thing we need to concern ourselves with is making a generic build that will work on the majority of linux-based systems regardless of distribution.

Distro specific packages, such as debs or rpms should be the problem of package maintainers downstream. I just happen to be a downstream developer for Debian (and via them Ubuntu), but I keep that totally separated from the work I do upstream. We'll get more millage out of having downstream handle their own packaging. :)

Is there no one maintaining the install/linux-install side of things? If not, then we can probably remove that in favor of a generic build/package nifskope.x.x.x.tar.gz solution.

In general, I'm against having binaries in source-control, like those found in install/win-install. It would be great if we could purge those (git history re-write) to trim down the size of the repo to keep it purely text. We can leverage appveyor to create packages for us and when we go to tag a release, we can just copy over the packages to our release. If we need to create exe/msi installer packages for nifskope, we can do that as well, though not in this PR. ;)

As for 'yet more files in root', I totally agree. Sadly our hands are tied here as travis and appveyor expect those files to be there. [1][2] I try very hard to keep those files as small, simple and easy to read as possible. However, if those files get too 'large' we can split out the scripts to a new CI directory or re-use the build directory and put CI under there. We're not there, yet. Our builds are thankfully very simple and our requirements are also small in comparison to larger projects I work on like OpenMW. ;)

fsengine was windows only because it included dds.h which included windows.h and dxgiformat.h who are by default only found on windows platforms. I found an license compatible solution for dxgiformat.h and added a bit to dds.h so fsengine should work without problems. I've tested the resulting build with lots of NIFs including ones 30MiB in size with lots of moving parts/animations/particles and textures. Everything seem to work like it does running a windows build under wine. If any issues come up, just assign the ticket to me and I'll have a look. I've not looked at the macro guards yet though, I'll give a look over but any results won't be in this PR.

I don't plan on doing any more work on this PR, I think it does its job in proving that we can build on the big 3 platforms. Any future work will land in other PRs, but they will likely require this one to go through first. :)

[1] https://docs.travis-ci.com/user/customizing-the-build/
[2] https://www.appveyor.com/docs/build-configuration/

neomonkeus · 2017-02-03T11:17:45Z

Are we happy to merge then.

psi29a · 2017-04-09T06:59:54Z

Ping? :)

hexabits · 2017-05-01T05:50:39Z

Sorry @psi29a :)

OK I finally stashed my local repo stuff and pulled PR 101.

Rebuilt in debug/release for msvc and in release for gcc. I decided testing for any kind of speed regression for fmin/fmax->std::min/max is simply not worth it as the former wasn't technically correct to begin with. That whole section will probably have to be redone in order to add BC7 support anyway.

@psi29a One last question about 15df8e4 so that I know for the future. Is this a required change because of explicit lack of SSH support from Travis/Appveyor or is it just easier to use HTTPS with these tools? Whatever the reason I want to have it ingrained so I stick to https syntax in the future.

Also @neomonkeus the "All checks have passed" details... Is this a repo setting I can't see? Just wondering why the Appveyor link goes to neomonkeus/nifskope and Travis says niftools/nifskope. I'll eventually need to know what to put in the README.MD for the build status badges.

psi29a · 2017-05-02T11:22:26Z

Well, for one the .gitmodules file wasn't consistent and that tickled my OCD a bit but the real problem is that I couldn't pull from those URLs so I made it consistent across all three with ones that did work.

In theory, it should work for you and me and others regardless. ;)

neomonkeus · 2017-05-02T14:02:31Z

@jonwd7 Good spot. I enabled it on my account instead of the niftools one. Corrected now.

neomonkeus · 2017-05-02T14:06:43Z

Ah, the problem is that even though AppVoyer knows about user/orgs. It is the user who is part of the org which will be referenced. Alternative is to create an account, but we don't have a corresponding one for github, as this is an org. Would have to convert everything to an user account which doesn't seem worth it.

psi29a · 2017-05-02T14:30:40Z

@neomonkeus we had this discussion already that AppVayer is retarded about user/orgs. Remember that whole game of letting me get access to your account? That is why I pused for a Niftools account instead of using your account.

darkbasic · 2019-09-16T16:58:19Z

@psi29a when did you check last time if it's still working?
Doesn't work for me and I'm unsure if it's because of my architecture (ppc64le) or because the Linux build broke in the meantime: #167

it finally compiles, removed windows.h, added additional typedefs and…

8e8b40d

… defines in addition to pulling in a gpl version of dxgiformat, also replaced tabs with spaces because tabs are evil ;) bring in dxgiformat

This was referenced Jan 12, 2017

windws.h {linux} hexabits/nifskope#12

Open

Does not build on Linux hexabits/nifskope#4

Closed

Building fails "collect2: error: ld returned 1 exit status" [dev/2.0] #90

Closed

neomonkeus requested a review from hexabits January 31, 2017 15:16

neomonkeus self-assigned this Jan 31, 2017

neomonkeus added c:build S:in-progress S:ready T:enhancement labels Jan 31, 2017

neomonkeus added this to the v2.0.0 milestone Jan 31, 2017

use std::min/max along with old style tabs in NifSkope.pro file

d5ebfc2

use https instead of git over ssh for submodules

15df8e4

psi29a force-pushed the 2.0.0-dev6-linux-fix branch from 465fdf9 to 3c9116c Compare February 1, 2017 12:23

tell docsys submodule to use master branch instead of particular commit

d8a26b9

add fsengine back in

psi29a force-pushed the 2.0.0-dev6-linux-fix branch from 3c9116c to d8a26b9 Compare February 1, 2017 14:02

psi29a added 9 commits February 1, 2017 16:28

branch is ignored in .gitmodule anyway

ee61d10

initial travis push

eccdeae

try to compile now turn on sudo and remove qt4

get me qt5 please

1637188

formatting try without debian-sid

try with qt551

9bfd59f

try getting qt5 from ppa try to get qt latest git

make nifskope on linux great again

7e3b2d8

try to trim time down and add clang

turn on osx building, try defaults first

0c7c808

try setting path for qt5 on osx build ;

give appveyor a spin

54fff33

go recursive build the damn thing try niftools#3 try to get a dir listing give me more output try vcxproj instead of sln try build dir try defaults fix bad yaml try building now

try collecting artifacts

b7ef142

try to clean up artifacts and introduce debug and release builds

c74f5cb

almost there try to get a nifskope dir case sensative try niftools#2 try niftools#3 get it right try build_name try hardcoded

hexabits approved these changes May 1, 2017

View reviewed changes

hexabits merged commit 7261b0a into niftools:develop May 1, 2017

psi29a mentioned this pull request Nov 5, 2017

NifSkope 1.1.3 for Linux OpenMW/openmw#1539

Closed

psi29a deleted the 2.0.0-dev6-linux-fix branch September 18, 2019 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows for compilation on Linux #101

Allows for compilation on Linux #101

psi29a commented Jan 12, 2017 •

edited

psi29a commented Jan 12, 2017 •

edited

hexabits commented Jan 31, 2017

MiroslavR commented Jan 31, 2017

hexabits commented Jan 31, 2017

psi29a commented Feb 1, 2017 •

edited

psi29a commented Feb 2, 2017

hexabits commented Feb 3, 2017

psi29a commented Feb 3, 2017 •

edited

neomonkeus commented Feb 3, 2017

psi29a commented Apr 9, 2017

hexabits commented May 1, 2017

psi29a commented May 2, 2017

neomonkeus commented May 2, 2017 •

edited

neomonkeus commented May 2, 2017

psi29a commented May 2, 2017

darkbasic commented Sep 16, 2019

Allows for compilation on Linux #101

Allows for compilation on Linux #101

Conversation

psi29a commented Jan 12, 2017 • edited

psi29a commented Jan 12, 2017 • edited

hexabits commented Jan 31, 2017

MiroslavR commented Jan 31, 2017

hexabits commented Jan 31, 2017

psi29a commented Feb 1, 2017 • edited

psi29a commented Feb 2, 2017

hexabits commented Feb 3, 2017

psi29a commented Feb 3, 2017 • edited

neomonkeus commented Feb 3, 2017

psi29a commented Apr 9, 2017

hexabits commented May 1, 2017

psi29a commented May 2, 2017

neomonkeus commented May 2, 2017 • edited

neomonkeus commented May 2, 2017

psi29a commented May 2, 2017

darkbasic commented Sep 16, 2019

psi29a commented Jan 12, 2017 •

edited

psi29a commented Jan 12, 2017 •

edited

psi29a commented Feb 1, 2017 •

edited

psi29a commented Feb 3, 2017 •

edited

neomonkeus commented May 2, 2017 •

edited