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

[NO_MERGE] Build on windows #8290

Closed
wants to merge 2 commits into from

Conversation

YongseopKim
Copy link
Contributor

Just try. Do not merge. /cc @mhs4670go

@YongseopKim YongseopKim added PR/NO TEST Tell CI to not run test PR/NO MERGE Please don't merge. I'm still working on this :) DRAFT A draft issue or PR for sharing one's current working status and discussion. labels Jan 18, 2022
@YongseopKim
Copy link
Contributor Author

Anyway, building is successful.

@@ -124,7 +124,7 @@ bool substitute_strided_slice_to_reshape(luci::CircleStridedSlice *ss_node)
std::bitset<32> end_mask(ss_node->end_mask());
std::bitset<32> shrink_axis_mask(ss_node->shrink_axis_mask());

uint input_rank = input_node->rank();
uint32_t input_rank = input_node->rank();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if(EXTRACTION_ERROR)
message(FATAL_ERROR "Extract ${PREFIX} - failed")
endif(EXTRACTION_ERROR)
# TODO:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -180,9 +181,16 @@ int entry(int argc, char **argv)

if (arser.get<bool>("--verbose"))
{
#if defined(__MINGW32__)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful if you add a comment that explains why you added this codes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we have never used compiler directives in our compiler frontend.
And I hope not to use them afterwards.
Reason: These make code readability hard and also development hard.

Copy link
Contributor Author

@YongseopKim YongseopKim Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your points :-) I'll make a draft for this with a detailed explaination.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, before the draft, let me explain shortly.

from https://sourceforge.net/p/mingw/mailman/message/35495004/

  1. MSVCRT.DLL exports putenv(), (strictly _putenv(), but MinGW defines
    an import library alias with the POSIX.1-2008-XSI compatible name),
    which is semantically similar to the Unix System V function of the
    same name. However, there is no setenv(), nor any semantically
    similar API, provided by any Windows DLL.

  2. Neither putenv(), nor setenv() is required by ISO-C, so, even
    though both are specified by POSIX, it isn't considered imperative
    for MinGW to provide what isn't already provided by MSVCRT.DLL.

The points are

  • setenv() is not implemented in MinGW because MinGW supports ISO-C spec.
  • putenv() can be used instead of setenv() (actually putenv() <- _putenv())

While building on windows, I've faced these problems.

/cc @mhs4670go and @seanshpark

Copy link
Contributor Author

@YongseopKim YongseopKim Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the draft, we need to discuss

  1. how much do we contribute to building on windows. I personally prefer working on windows env so I'm doing this task. But, ONE officially doesn't support Windows. How much do we contribute to windows? (of course, I can contribute this as a personal developer NOT an official ONE developer.)
  2. Anyway, if I do this task, how to do it? @llFreetimell suggests making a new directory like WinUtil and wrapping functions like setenv().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think would be better to post a separate issue for this --> #8308

Copy link
Contributor

@seanshpark seanshpark Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides supporting for Windows, I don't think I will accept codes using like #if defined(__MINGW32__). As I wrote, introducing this one will eventually permit codes like this and that will lead to hell. (which I had experienced long time ago that I don't want to experience again)

You can consider supporting multi platform as, provide PAL (platform adaptation layer), detect/select platfrom from CMake.

@@ -33,7 +33,7 @@
#include <sstream>
#include <string>
#include <vector>
#include <cstdlib>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@mhs4670go
Copy link
Contributor

If mingw and linux have mutually exclusive things, it is good to use #define(directives) but if not, there seems to be few options.

  • Use another libraries(functions) that do the same thing with current one that exist in both system.
  • Change the logic to another one that uses libraries that exist in both system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DRAFT A draft issue or PR for sharing one's current working status and discussion. PR/NO MERGE Please don't merge. I'm still working on this :) PR/NO TEST Tell CI to not run test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants