Skip to content
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

Windows porting (MinGW) #75

Merged
merged 5 commits into from
Sep 15, 2020
Merged

Windows porting (MinGW) #75

merged 5 commits into from
Sep 15, 2020

Conversation

roccoarpa
Copy link
Contributor

I'm opening temporarily this pull request to track down the developments for bitpit's Windows porting through msys2/mingw.
Personally, I've fully tested it on local Win10 machines, and it works. I've just some doubts on the final expected results of some modules examples, but i think it's something related to my lack-of-knowledge.
@edoardolombardi @marcocisternino, as far as you know, before officially asking the final review/merge to @andrea-iob , do we need more testing on this branch?

Let me know your opinion, just to decide how to proceed.

@andrea-iob andrea-iob changed the title General.windowsmingw.porting Windows porting (MinGW) Jul 15, 2020
@edoardolombardi
Copy link
Member

Even on my machine compilation and testing are ok.

@marcocisternino
Copy link
Member

marcocisternino commented Jul 16, 2020 via email

src/IO/VTK.cpp Show resolved Hide resolved
src/IO/VTK.cpp Outdated Show resolved Hide resolved
src/IO/VTK.cpp Show resolved Hide resolved
cmake/CMakeLists.txt Outdated Show resolved Hide resolved
README_WINDOWS.md Show resolved Hide resolved
@roccoarpa roccoarpa force-pushed the general.WindowsMinGW.porting branch from 059d285 to de7db05 Compare July 17, 2020 07:27
@andrea-iob
Copy link
Member

There is one more issue I would like to address before merging the branch into the master (I'm not sure if I've already reported it in the review). I think it's better to remove the duplicates from the variable BITPIT_EXTERNAL_LIBRARIES itself, rather than creating a new variable. Also, I'm not sure if the function REMOVE_DUPLICATES works properly in this case. If the list of external libraries is something like "library_1.a libmpi.a library_2.a libmpi.a", removing the duplicates would give us "library_1.a libmpi.a library_2.a", but this will not work if library_2.a depend on libmpi.

@roccoarpa
Copy link
Contributor Author

There is one more issue I would like to address before merging the branch into the master (I'm not sure if I've already reported it in the review). I think it's better to remove the duplicates from the variable BITPIT_EXTERNAL_LIBRARIES itself, rather than creating a new variable. Also, I'm not sure if the function REMOVE_DUPLICATES works properly in this case. If the list of external libraries is something like "library_1.a libmpi.a library_2.a libmpi.a", removing the duplicates would give us "library_1.a libmpi.a library_2.a", but this will not work if library_2.a depend on libmpi.

Look, I did some further tests and I realized this REMOVE_DUPLICATES story was really unnecessary to land the commit. So nevermind. I will push an updated version shortly. Hope it satisfies your requests.

@andrea-iob andrea-iob force-pushed the general.WindowsMinGW.porting branch 3 times, most recently from f14cba4 to 10ad61c Compare September 9, 2020 13:33
@andrea-iob
Copy link
Member

In my opinion, the problem with the PETSc static library will arise also for all other static variables. I think it would be better to remove all static variables from the list of external libraries exported by bitpit. I've added this change in commit c8aea1d. I've also moved the definition of PETSC_ARCH and PETSC_DIR before finding PETSc package (13463d9).

@marcocisternino confirms that the updated branch compiles on Windows.

If @roccoarpa confirms that the changes are fine we can merge into master.

@roccoarpa
Copy link
Contributor Author

In my opinion, the problem with the PETSc static library will arise also for all other static variables. I think it would be better to remove all static variables from the list of external libraries exported by bitpit. I've added this change in commit c8aea1d. I've also moved the definition of PETSC_ARCH and PETSC_DIR before finding PETSc package (13463d9).

@marcocisternino confirms that the updated branch compiles on Windows.

If @roccoarpa confirms that the changes are fine we can merge into master.

From my side all I need is that the full, shared(dll.a or dll) compiled bitpit can be included in a external project who explicitly uses a cmake find_package(BITPIT) command to retrieve it, without arising dependencies conflicts. If your new modifications/commits achieve it, they are fine for me. Regards.

@andrea-iob andrea-iob self-requested a review September 9, 2020 15:20
@andrea-iob andrea-iob added this to the 1.7 milestone Sep 12, 2020
roccoarpa and others added 5 commits September 15, 2020 08:45
The commit addressed the problem of stream repositioning with
seekg/tellg when the stream is opened in full text mode(ascii). Because
of different escaping chars w.r.t to Unix, seekg/tellg on ascii file
under Windows do not work as presumed in bitpit::VTK code.
The trick is to open always the file in binary mode: the last works both
in win and unix/linux.

See notes for developers left as code comment, for usage examples.
@andrea-iob andrea-iob merged commit bbf2a6d into master Sep 15, 2020
@andrea-iob andrea-iob deleted the general.WindowsMinGW.porting branch September 15, 2020 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants