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

minlib tech with optimized rebuild #477

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davidbabono
Copy link

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)

Copy link
Collaborator

@gparmer gparmer left a comment

Choose a reason for hiding this comment

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

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?

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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

@@ -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)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

# 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))
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this was an accident, undo it.

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

let mut cflag_minlib = "-ffunction-sections -fdata-sections";

//rebuild the files under the related sub directories
let output = Command::new("make")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

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

program_name.unwrap()
));
}

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

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove debugging print statements, and format.

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

Choose a reason for hiding this comment

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

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

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.

2 participants