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

Modified build system #55

Merged
merged 1 commit into from
Sep 4, 2018
Merged

Modified build system #55

merged 1 commit into from
Sep 4, 2018

Conversation

talshadow
Copy link
Contributor

This is my small proposition about modification of build system

Copy link
Contributor

@wjhun wjhun left a comment

Choose a reason for hiding this comment

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

It looks like you removed def32.h, def64.h, combined them into uniboot.h, and then added that include to every source file.

I don't understand. How is this an improvement to the build system?

@talshadow
Copy link
Contributor Author

Yeah, also unix_process.h and stage3.h will be removed.
From my point of view this change do separating code dependencies from build system.
i think use directive -include good decision only for precompile headers.
Second goal is improveing by IDEs code recognition.
p.s. I use QtCreator ... What IDE are you use?

@convolvatron
Copy link
Contributor

I'm with William, I dont see whats going on here. Is the goal to change

cc -include foo.h ..
to
cc -DFOO ...
uniboot.h:
#ifdef FOO
....
#endif
etc...

I dont see where this configuration directory gets added...I supposed I'll check it out and see how it builds

@convolvatron
Copy link
Contributor

ok, so you dislike -include so much you put the two cases in a include include with with an ifdef.

is it possible to still have one file per architecture in the respective architecture directory?

@talshadow
Copy link
Contributor Author

talshadow commented Sep 3, 2018

I think I can do this but partial. I can put files with same name to each architecture dir, but we still use two files with very similar content I mean the unix_process.h and stage3.h
As variant we can use uniboot.h which will be select which header we should include per architecture

@convolvatron
Copy link
Contributor

maybe this will help. aside from removing support for -include which isnt working well with your IDE, what are you trying to accomplish here?

the original goal was to allow, at build time, parameterization of the running environment (i.e. the manner in which dynamic types are tagged, the definition of fixed sized types, the implementation of halt, etc) in a manner that didn't require changing the source or throwing #ifdefs everywhere.

your (update) proposal is to replace -include with uniboot.h which
does
#ifdef PLATFORM_A
#include "platforma.h"
#endif
#ifdef PLATFORM_B
...

if so I'd prefer we -D PLATFORM_A on the command line as you did in at least one example instead of depending on cpp definitions defined by default.

if so, then its pretty much exactly the same as it is now. if that makes it easier for you in some way, sure why not?

@talshadow
Copy link
Contributor Author

talshadow commented Sep 3, 2018

Yes, this may help me. But right now I can determine i386 and x32_64 in compile time (which i do in uniboot.h only one ifdef) but i can't determinate 2 case - differ between stage3.h and unix_process.h. This files have same content and differ is only in one macro. For one of this case i use value by default other case i use -DSTAGE3 directive

@wjhun
Copy link
Contributor

wjhun commented Sep 3, 2018

I think it shouldn't be necessary to determine anything at compile time but instead to depend on Makefile defines. Simply put, e.g.,

(use CFLAGS)
boot/Makefile defines -DBITS32 (-DDEF32?) and -DSTAGE2 for the stage2 targets
stage3/Makefile defines -DBITS64 and -DSTAGE3
unix_process/Makefile defines -DBITS64 and -DUNIX_RUNTIME

Given how the build system is organized, there should be no need to try and depend on cpp built in defs.

Also, instead of adding "uniboot.h" to every source file, could you add it to runtime.h so that we can maintain the convention of only including a single source file? I am in favor of keeping the current convention rather than starting to require more and more headers...

@talshadow
Copy link
Contributor Author

The options -m32 -m64 of compiler already generated the predefined macros which help us select target system bit capacity.
Modification of 'runtime.h' looks as very nice - i will this changes.

@SergiusTheBest
Copy link

@convolvatron The main reason for this change is to allow IDE to know which files are included as -include goes behind it.

@wjhun
Copy link
Contributor

wjhun commented Sep 3, 2018

Ok, I'm cool with getting rid of the -include lines. That may help in calculating header dependencies as well. (#48)

@convolvatron
Copy link
Contributor

ok, I think we're close. I agree with William that we may as well fold this into runtime.h

if we get rid of the -include lines can we please define our own platform variables
(i.e. BITS32 or whatever) instead of using whatever the compiler has set up?

otherwise I'm good

@talshadow
Copy link
Contributor Author

talshadow commented Sep 3, 2018

What IDE or editors are you use?

@convolvatron
Copy link
Contributor

I kind of dont understand why thats relevant, surely unless we enforce everyone using the same IDE, we should pretty much avoid making the environment be IDE specific.

I understand how the -include thing is a more general problem since it sneaks in dependencies/references that maybe the IDE wasn't prepared to look for.

I use emacs

@wjhun
Copy link
Contributor

wjhun commented Sep 3, 2018

emacs in gnu screen here

@talshadow
Copy link
Contributor Author

I asked about IDE just from curiousness. I thought that will have been vim .. I was mistake.
As for me it is not only IDE problem - when I look in file i can't determine dependencies if we determinate some of it through '-include'. At the now I should determine the most of them because I not so familiar with current code base.

@talshadow
Copy link
Contributor Author

@convolvatron , @wjhun
Gentlemen please look again. Is it better now from your point of view?

Copy link
Contributor

@wjhun wjhun left a comment

Choose a reason for hiding this comment

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

This looks good. Thank you.

@talshadow talshadow merged commit 20a7676 into master Sep 4, 2018
@eyberg eyberg deleted the feature/update_buildsystem branch October 28, 2018 18:42
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.

None yet

4 participants