Skip to content
This repository was archived by the owner on Mar 24, 2026. It is now read-only.

leda micro framework#887

Merged
aschneider-techempower merged 35 commits intomasterfrom
unknown repository
Jul 21, 2014
Merged

leda micro framework#887
aschneider-techempower merged 35 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jun 30, 2014

Please accept the basic test support for leda framework (https://github.com/sergeyzavadski/leda)

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 10, 2014

Is this all that is needed to add framework to tests?

@aschneider-techempower
Copy link
Copy Markdown
Contributor

Because of #900, we also require inclusion of the install.sh file. Here is an example of another LUA framework.

If you need to download a framework, which it looks like you do, you also need to do, you need to provide a script in the toolset/setup/linux/frameworks directory.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 10, 2014

Thanks, I have updated the pull request.

leda/instasll.sh Outdated
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

leda is the same concept as node.js and node.js is in languages directory. following that logic node.js should be in the frameworks directory. i will rename the file and move it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The file should be in toolset/setup/linux/frameworks/leda.sh, not languages. Please keep the install.sh file consistent with the rest of the frameworks.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 10, 2014

Does it look good now?

leda/app.lua Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't there else clause in Lua ?
There is no need to check for /plaintext in the url if /json has been processed earlier.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

thanks, I have updated the code

leda/app.lua Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

another (minor) optimization is to cache the url and not call request:url() twice in the function body
if it is simple getter then it is really minor thingy, but if there is some logic involved then the caching would help

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

thanks, I have added that as well.

@aschneider-techempower
Copy link
Copy Markdown
Contributor

That doesn't appear to have worked

@hamiltont mentioned he may be able to help out later. He's the one that engineered the entire setup process.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 18, 2014

So it is failing because it does not build libevent before the code that depends on it. Looks like building is split between child processes and synchronization fails, since libevent has to be built explicitly first.

Is there a way to synchronize targets when building? in that case libevent has to built before the lib propeller target

And there is this warning:
make[2]: warning: jobserver unavailable: using -j1. Add `+' to parent make rule.

@hamiltont
Copy link
Copy Markdown
Contributor

Are you saying that running make does not ensure that libevent is built before it is used? If that's the case, you will need to fix that yourself-Perhaps there's some environment variable you can declare before you run make that will force serial compilation of the necessary bits? I'm not sure I understand what you mean by 'synchronize targets'

Some other advice - before you finish this, could you please see the "guidelines" section of https://github.com/TechEmpower/FrameworkBenchmarks/tree/master/toolset/setup/linux for items 3 and 4, you are currently violating both of those. Also, please use fw_untar instead of tar xvf, so that we can later update everything by just updating fw_tar if we want to

@hamiltont
Copy link
Copy Markdown
Contributor

Or perhaps you need to prefix your configure/make with sudo apt-get install libevent-dev? Obviously it's preferred to download the libevent sources and build from scratch, but if that's not an option just use apt-get

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 19, 2014

The makefile line that is failing looks like this:
$(TARGET): libevent $(OBJECTS)
and the problem is that it is starting to build $(OBJECTS) before libevent is built. What are you using to process makefile targets? There must be a way to force targets order.

Linking libevent dynamically is not the option unfortunately since I am using version more recent than in distribution

I have made the changes you suggested

@hamiltont
Copy link
Copy Markdown
Contributor

I believe we use cmake. Look in prerequisites.sh to double check

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 19, 2014

@hamiltont
Copy link
Copy Markdown
Contributor

Do you have charge of the makefile? If so, I'd recommend against turning
off parallel. You probably do want parallel compilation for your
$(objects). If there are a number of objects that's a big speedup. The
right fix would be to rewrite the makefile so libevent was always compiled
first by making objects depend on libevent explicitly
On Jul 18, 2014 8:09 PM, "Sergey Zavadski" notifications@github.com wrote:

I see it, https://www.gnu.org/software/make/manual/html_node/Parallel.html.


Reply to this email directly or view it on GitHub
#887 (comment)
.

@hamiltont
Copy link
Copy Markdown
Contributor

Eg if there is a dependency, that should be outlined in the makefile. If
you just turn off parallel but don't express the dependency, you still have
no guarantee that libevent will be compiled before the other items on that
line, right?
On Jul 18, 2014 8:09 PM, "Sergey Zavadski" notifications@github.com wrote:

I see it, https://www.gnu.org/software/make/manual/html_node/Parallel.html.


Reply to this email directly or view it on GitHub
#887 (comment)
.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 19, 2014

The thing is that I do specify dependency it GNU make that is not honoring it and starts building all targets in parallel. So the only option is to disable parallel build. That will slow down the build process but will result in correct order

that is the only choice I see, I have updated the source

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 19, 2014

I wish make had better controls for target dependency management during parallel builds..

@hamiltont
Copy link
Copy Markdown
Contributor

Perhaps I'm confused (on mobile, cant easily see your updates) but you
should do this:

FinalProduct: objects
    ...stuff
Objects: libevent
    ...stuff

This is declaring a dependency, nothing else. AFAIK, the order of the items
after the colon provides no guarantee about compilation order. So the
makefile i saw does not declare that $objects depends on libevent--- you
are just getting lucky that your specific setup interprets things that way.
If you want a portable makefile, you need to declare the dependencies
properly.

This has nothing to do with parallel or not parallel. Even worse than
slowing down your compile by a factor of 8 (on the test machines), no
parallel doesn't guarantee a successful compilation...it just makes the bug
less frequent.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 19, 2014

no, it is exactly the issue that libevent target is being built in parallel with others, i have reproduced the same error when invoking make with parallel flag

the example you gave does not work.. OBJECTS is macro that resolves to a list, it is not a single file
I get errors like make: src/Buffer.o: Command not found

the build is slower but results in the correct executable

What options do we have here? All other targets in the build can be built concurrently, but make gives all or nothing

@hamiltont
Copy link
Copy Markdown
Contributor

The above was just pseudocode. You've updated your makefiles a lot recently so it took a bit to track this down. As you already spotted, the issue is in the libpropeller Makefile. To 100% ensure that libevent is built before any of the object files are constructed, just declare it as a dependency:

%.o: %.cpp
        $(CXX) -c -o $@ $(PROPELLER_CXXFLAGS) $(CPPDEPS) $<

Into this:

%.o: %.cpp libevent
        $(CXX) -c -o $@ $(PROPELLER_CXXFLAGS) $(CPPDEPS) $<

Also, please change

libevent:
        cd deps/libevent && make

back to

libevent:
        cd deps/libevent && $(MAKE)

so make can pass through all flags (including -j). Now you should be able to build with make -j8 or otherwise with no issues. Even in parallel mode, make respects any dependencies you have listed. If this doesnt' work for you for some reason, please provide your exact make command and a download to the exact code that you are using

@hamiltont
Copy link
Copy Markdown
Contributor

PS - nice job updating based on the guidelines so far. You will want to also change rm -rf leda to rm -rf leda || true based on guideline 5 (the annoyingly tricky one...)

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 19, 2014

that will not work either.. through includes the missing file gets included in the main leda target as header. It is not just lib propeller but everything that uses lib propeller as well

the main issue is in event2/event-config.h file that gets generated during libevent build process and everything depends on it, thats why it has to very first target built (there is also leda source that depends on this file as well)

Ideally that file should be generated in configure stage. I will see if I can update that

I have updated the bash script

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 19, 2014

I can just move libevent build to configure.. that will not be the cleanest way but will solve all issues

@hamiltont
Copy link
Copy Markdown
Contributor

If you would like to join me on IRC, we can fix up your Makefiles. I'm 95% sure this can be resolved perfectly normally inside a makefile. After all, they predominantly exist to declare dependencies!

http://webchat.freenode.net/?channels=techempower-fwbm for the channel location

@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 19, 2014

I have moved libevent build to configure and that should solve all issues, make -j will work normally
This issue is not contained within a single makefile as it spans across different makefiles. Synchronizing build phases by moving it to a different script is fine solution (and I have seen it done in other projects). libevent build is a short one anyways

Thanks for help

@hamiltont
Copy link
Copy Markdown
Contributor

Sounds good!

aschneider-techempower added a commit that referenced this pull request Jul 21, 2014
@aschneider-techempower aschneider-techempower merged commit e4782d6 into TechEmpower:master Jul 21, 2014
@aschneider-techempower
Copy link
Copy Markdown
Contributor

Thanks for all your effort for trying to merge the PR @hamiltont and @sergeyzavadski

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants