minlib tech with optimized rebuild #477

davidbabono · 2024-01-21T05:05:30Z

Summary of this Pull Request (PR)

Add description here.

Intent for your PR

Choose one (Mandatory):

This PR is for a code-review and is intended to get feedback, but not to be pulled yet.
This PR is mature, and ready to be integrated into the repo.

Reviewers (Mandatory):

(Specify @<github.com username(s)> of the reviewers. Ex: @user1, @user2)

Code Quality

As part of this pull request, I've considered the following:

Style:

Comments adhere to the Style Guide (SG)
Spacing adhere's to the SG
Naming adhere's to the SG
All other aspects of the SG are adhered to, or exceptions are justified in this pull request
I have run the auto formatter on my code before submitting this PR (see doc/auto_formatter.md for instructions)

Code Craftsmanship:

I've made an attempt to remove all redundant code
I've considered ways in which my changes might impact existing code, and cleaned it up
I've formatted the code in an effort to make it easier to read (proper error handling, function use, etc...)
I've commented appropriately where code is tricky
I agree that there is no "throw-away" code, and that code in this PR is of high quality

Testing

I've tested the code using the following test programs (provide list here):

micro_booter
unit_pingpong
unit_schedtests
...(add others here)

gparmer

I'm missing where the dependencies are queried in the composer, then used to compile the libs/interfaces. Am I missing it, or is it still TODO?

gparmer · 2024-02-08T20:21:21Z

cos

+        src/composer/target/debug/compose $script $name
+		local dir="system_binaries/cos_build-${name}"
+    else
+        echo "[cos executing] src/composer/target/debug/compose $script $name $minlib"


I don't think you need to replicate the echo here. If you simply print it out before the conditional, if $minlib is nothing, it won't print out anything. Which should be identical in behavior to this code, without the replication. I might be wrong.

gparmer · 2024-02-08T20:25:47Z

src/components/implementation/Makefile

@@ -8,6 +8,7 @@ all:
 	$(info ***********************************************)
 	$(info *********[ Building Implementations ]**********)
 	$(info ***********************************************)
+	$(info $(SUBDIRS))


This is likely just for debugging, I'm assuming. If so, it should not appear in a PR.

gparmer · 2024-02-08T22:08:04Z

src/components/implementation/Makefile.subsubdir

@@ -79,28 +79,37 @@ COMP_DEPLIBDIRS_CLEAN=$(foreach D,$(COMP_DEPS_CLEAN),$(if $(wildcard $(INTERDIR)
 COMP_EXPIF_OBJS=$(foreach I,$(COMP_INTERFACES_CLEAN),$(INTERDIR)/$(I)/cosrt_s_stub.o)
 COMP_DEP_OBJS=$(foreach D,$(COMP_IFDEPS_CLEAN),$(INTERDIR)/$(D)/cosrt_c_stub.o)

+DEP_DIRS := $(foreach obj,$(COMP_DEP_OBJS),$(dir $(obj)))


Maybe a comment block above these to say what they are trying to do?

gparmer · 2024-02-08T23:56:47Z

src/components/implementation/Makefile.subsubdir

 # NOTE: we're currently ignoring the *variants* library requirements,
 # which will break if an interface's code requires a library

 comp_header:
 	$(info | Composing $(COMP_INTERFACE).$(COMP_NAME) for variable $(COMP_VARNAME) by linking with:)
 	$(info |     Exported interfaces: $(COMP_INTERFACES_CLEAN))
 	$(info |     Interface dependencies: $(COMP_IFDEPS_CLEAN))
-	$(info |     Libraries: $(DEPENDENCY_LIBS) $(DEPENDENCY_LIBOBJS))
+	$(info |     Libraries:$(DEPENDENCY_LIBS) $(DEPENDENCY_LIBOBJS))


If this was an accident, undo it.

gparmer · 2024-02-09T00:09:12Z

src/components/implementation/comp_x86_64.ld

@@ -11,7 +11,7 @@ SECTIONS
 	. = SIZEOF_HEADERS;

 	/* start the text/read-only sections */
-	.text : ALIGN(4096) { *(.text*) } :text
+	.text : ALIGN(4096) {	    KEEP(*(.text.__cosrt_c_cosrtdefault))	*(.text*) } :text


I don't understand the spacing on this. Feel free to use multiple lines if that works with the syntax here.

.text : ALIGN(4096) { KEEP(*(.text.__cosrt_c_cosrtdefault)) *(.text*) } :text

My concern with this is this: if this is necessary to keep symbols around, don't we need an automated way to construct all of the symbols, not just the default entry point?

gparmer · 2024-02-09T01:59:27Z

src/composer/src/build.rs

+            let mut cflag_minlib = "-ffunction-sections -fdata-sections";
+
+            //rebuild the files under the related sub directories
+            let output = Command::new("make")


Should be able to use exec_pipeline here instead of pulling in the dependency on Command, right? I think you explained why you couldn't, but I can't figure out now.

Coming back to this now. I'm guessing you need the actual output, and exec_pipeline doesn't provide that. I think that the correct abstraction level is to provide an exec_pipeline variant that returns (stdout, stderr) output strings.

gparmer · 2024-02-09T02:02:07Z

src/composer/src/build.rs

+
+            if output.status.success() {
+                let stdout = String::from_utf8_lossy(&output.stdout);
+                println!("Output: {}", stdout);


These seem like they are focused on debugging output? not sure if we really want this output normally.

gparmer · 2024-02-09T11:39:23Z

src/composer/src/main.rs

            program_name.unwrap()
        ));
    }

+    let b_minlib= arg3.as_ref().map(|s| s == "minlib").unwrap_or(false);


Not sure what the b_ stands for here. Maybe use_minlib?

gparmer · 2024-02-09T11:40:11Z

src/composer/src/syshelpers.rs

@@ -6,9 +6,11 @@ use std::fs;
 pub fn exec_pipeline(progs: Vec<String>) -> (String, String) {
    let output = progs.iter()
        .fold(None,
-              |upstream: Option<Pipe>, cmd| match upstream {
+              |upstream: Option<Pipe>, cmd| { println!("Executing command: {}", cmd); // This line added to print the command


Please remove debugging print statements, and format.

gparmer · 2024-02-09T11:42:58Z

src/composer/src/syshelpers.rs

@@ -6,9 +6,11 @@ use std::fs;
 pub fn exec_pipeline(progs: Vec<String>) -> (String, String) {


OK, so this already does return the outputs. So it might work instead of using Command directly?

minlib tech with optimized rebuild

d3f00c9

gparmer reviewed Feb 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

minlib tech with optimized rebuild #477

minlib tech with optimized rebuild #477

davidbabono commented Jan 21, 2024

gparmer left a comment

gparmer Feb 8, 2024

gparmer Feb 8, 2024

gparmer Feb 8, 2024

gparmer Feb 8, 2024

gparmer Feb 9, 2024

gparmer Feb 9, 2024

gparmer Feb 9, 2024

gparmer Feb 9, 2024

gparmer Feb 9, 2024

gparmer Feb 9, 2024

gparmer Feb 9, 2024

gparmer Feb 9, 2024

		@@ -6,9 +6,11 @@ use std::fs;
		pub fn exec_pipeline(progs: Vec<String>) -> (String, String) {

minlib tech with optimized rebuild #477

Are you sure you want to change the base?

minlib tech with optimized rebuild #477

Conversation

davidbabono commented Jan 21, 2024

Summary of this Pull Request (PR)

Intent for your PR

Reviewers (Mandatory):

Code Quality

Testing

gparmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment