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

Cmake support #116

Closed
ghost opened this issue Aug 30, 2017 · 18 comments
Closed

Cmake support #116

ghost opened this issue Aug 30, 2017 · 18 comments

Comments

@ghost
Copy link

ghost commented Aug 30, 2017

Hello,

would it be possible to write Cmake build file as it is easier to integrate this library as submodule in project.

Thanks in advance.

@lexborisov
Copy link
Owner

Hi!

I'll think about it. Now it's a modular system, and it needs to be supported in cmake.
It is not difficult at all, but it will take some time.

@ghost
Copy link
Author

ghost commented Sep 12, 2017

Hello,
i have added the cmake support in #120. Please merge the changes so it is easier to integrate your project as submodule.

@lexborisov
Copy link
Owner

Hi @brano543

Thanks! Merged!
It remains to support the arguments https://github.com/lexborisov/myhtml/blob/master/Makefile#L21

You can see previous cmake file (removed 21 Mar)

And there it is necessary to maintain the modularity of the system
This is not good. We must connect all modules in the directory source/* (myhtml, mycore and other new or removed)
To avoid changing file every time (CMakeLists.txt) directory at changing the directory source

@lexborisov
Copy link
Owner

And for a Linux we need add add_definitions(-D_POSIX_C_SOURCE=199309L)

@ghost
Copy link
Author

ghost commented Sep 13, 2017

I don' t know what am i doing wrong, but when i add_subdirectory, and set up includes, even in target_link_library i add myhtml, still i get undefined reference errors....

@lexborisov
Copy link
Owner

That is, your variant of CMakeLists.txt does not work?
So it must be remade or deleted from the project (until the working version appears).
I could deal with this, but so far this is not a priority for me. If you figure this out will be great.

@ghost
Copy link
Author

ghost commented Sep 14, 2017

i will try, but i don't know why i get unreferenced symbols, i am sure i include all of the files and the library is linked correctly.

@ghost
Copy link
Author

ghost commented Sep 14, 2017

I have fixed the cmake build. I didn' t notice i was overriding the variable 3 times, because i didn't put it into single command. #121

@lexborisov
Copy link
Owner

Merged!
But the question of modularity remained open.

@ghost
Copy link
Author

ghost commented Sep 14, 2017

The problem is it is difficult to tell cmake to exclude that just one port directory, because you have to choose between the OS. If i just include both of them, it won' t compile, because there will be same definitions for the ported functions.

lexborisov added a commit that referenced this issue Sep 15, 2017
@lexborisov
Copy link
Owner

@brano543
that's what I meant

@ghost
Copy link
Author

ghost commented Sep 16, 2017

Great job, i just had to fix one typo in the path for ports. You have accidentally went one directory upwards from current cmake source directory. I have fixed that in #122 and it is building just fine

@lexborisov
Copy link
Owner

Have we solved this problem?
At the moment, the cmake (CMakeLists.txt) is available.

@ghost
Copy link
Author

ghost commented Jan 4, 2018

Hello, I see you have made some changes to the CMakeLists.txt. I will update version of the submodule in my project and let you know if it still works. I have quickly reviewed the changes and it should, but may I ask you for one change though? Could you please rename the arguments
PROJECT_BUILD_SHARED
PROJECT_BUILD_STATIC
PROJECT_INSTALL_HEADER
to lets say
MYHTML_BUILD_SHARED
MYHTML_BUILD_STATIC
MYHTML_INSTALL_HEADER
so it is more specific to this project and prevent ambiguity between project that may use similar naming?

@lexborisov
Copy link
Owner

Hi!
Ok, no problem, I'm renaming these variables.

@ghost
Copy link
Author

ghost commented Jan 5, 2018

Ok, after you rename I will test it.

lexborisov added a commit that referenced this issue Jan 7, 2018
@lexborisov
Copy link
Owner

@brano543
I renamed this variables, please try it

@ghost
Copy link
Author

ghost commented Jan 7, 2018

Thank you for changing those variables ! It is working fine, thank you for doing such a great job in improving the CmakeLists I made.

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

No branches or pull requests

1 participant