You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by caguado <gi...@git.apache.org> on 2015/06/03 12:53:36 UTC
[GitHub] trafficserver pull request: TS-3607: Fix ats_pagespeed to use stdi...
GitHub user caguado opened a pull request:
https://github.com/apache/trafficserver/pull/210
TS-3607: Fix ats_pagespeed to use stdint.h macros
This commit adds _STDC_LIMIT_MACROS and __STDC_CONSTANT_MACROS to
fix the missing macros defined in stdint.h when building a C++ source
code.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/caguado/trafficserver fix/pagespeed_build
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/210.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #210
----
commit 6db71f944e3a2df660d0fd58b1bf5f7c793bf011
Author: Carlos Aguado <ca...@gmail.com>
Date: 2015-06-03T10:51:54Z
TS-3607: Fix ats_pagespeed to use stdint.h macros
This commit adds _STDC_LIMIT_MACROS and __STDC_CONSTANT_MACROS to
fix the missing macros defined in stdint.h when building a C++ source
code.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-219186527
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-218525702
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-219194934
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Fix ats_pagespeed to use stdi...
Posted by oschaaf <gi...@git.apache.org>.
Github user oschaaf commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-108486656
@jpeach Because the pagespeed libraries currently do not support all the platforms that ATS does, so some work has to be done to exclude the plugin in some platform builds.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Fix ats_pagespeed to use stdi...
Posted by caguado <gi...@git.apache.org>.
Github user caguado commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-110008880
How does it look? I'm quite new in writing autotools. Is it ok to rename gzip.so to ats_pagespeed_gzip.so? Documentation would need an update too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-219190165
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-219191881
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-219262163
This unfortunately now runs into issues with clang-format. Can you rebase, and run clang-format again, and then force push? See
https://ci.trafficserver.apache.org/view/github/job/Github-Linux/70/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Fix ats_pagespeed to use stdi...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-108485861
Wait, why isn't page speed integrated into the automake build? We add these macros to the build globally at configuration time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-218782640
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #210: TS-3607: Integrate ats_pagespeed into the automake...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the issue:
https://github.com/apache/trafficserver/pull/210
Please reopen or create another pull request if you want to resolve this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-199999165
Easiest is to look at the Jira, which has instructions how to merge and push. Make sure you "git pull --rebase" before you push upstream to ASF, otherwise the "Committer" field isn't update properly. Basically, we want the author to be the person submitting the PR, but you as the committer should show up as well (so we can see who actually pushed it).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Fix ats_pagespeed to use stdi...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-108487426
That's fine, we have other stuff that only builds when it's dependencies are available. I'm happy to advise if you want help doing this in autotools.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by oschaaf <gi...@git.apache.org>.
Github user oschaaf commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-218528146
The patch looks good to me, but @jpeach had an improvement in mind. @jpeach would you be OK with committing this and doing https://github.com/apache/trafficserver/pull/210#discussion_r57108812 in a follow-up?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by oschaaf <gi...@git.apache.org>.
Github user oschaaf commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-200003777
@zwoop thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Fix ats_pagespeed to use stdi...
Posted by oschaaf <gi...@git.apache.org>.
Github user oschaaf commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-110010001
@caguado my 2p is that renaming the specific gzip module for ats_pagespeed is fine for now.
From the top of my head the gzip version from ats_pagespeed is nearly identical to the real one.
I will have a look at merging and eliminating this gzip plugin for later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-199859557
@oschaaf Ping!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by caguado <gi...@git.apache.org>.
Github user caguado commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-220460158
Rebased against current 5.3.x and passed clang-format.
@jpeach, please let me know whether this is what you had in mind.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-218585959
Sure either way is fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by oschaaf <gi...@git.apache.org>.
Github user oschaaf commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-199991263
Change looks good to me! What is the flow with regard to committing a pull request?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-220376341
.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-112875075
https://issues.apache.org/jira/browse/TS-3607
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Fix ats_pagespeed to use stdi...
Posted by oschaaf <gi...@git.apache.org>.
Github user oschaaf commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-108296934
Great! LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by oschaaf <gi...@git.apache.org>.
Github user oschaaf commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-188185193
@bryancall yes. We've got some changes in our pipeline as well, I'll follow up shortly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #210: TS-3607: Integrate ats_pagespeed into the automake...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/210
We still need an update to this. @oschaaf and @jpeach , assigning to you for final reviews, but we should try to land this assuming it seems reasonable (once clang-format etc. has been addressed).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-187799694
@oschaaf can you please work on this and get it committed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #210: TS-3607: Integrate ats_pagespeed into the automake...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the issue:
https://github.com/apache/trafficserver/pull/210
I haven't seen an update to this PR for awhile. I will close it in a week if it hasn't been updated.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-217466804
Ping @oschaaf . Also, Otto, you must do the git-dual setup steps to make your account properly enabled here, otherwise, we can't assign PR's to you.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #210: TS-3607: Integrate ats_pagespeed into the a...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall closed the pull request at:
https://github.com/apache/trafficserver/pull/210
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/210#discussion_r57108812
--- Diff: plugins/experimental/ats_pagespeed/Makefile.am ---
@@ -0,0 +1,72 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+include $(top_srcdir)/build/plugins.mk
+
+if ATS_PAGESPEED_LINUX
+ os_name = linux
+else
+ os_name = unknown_os
+endif
+
+if ATS_PAGESPEED_X86_64
+ arch_name = x64
+else
+ arch_name = ia32
+endif
+
--- End diff --
You don't need to do this. In ``configure.ac`` changes, just ``AC_SUBST`` the values. Then later on you can use ``@ATS_PAGESPEED_OS@`` and ``@ATS_PAGESPEED_ARCH@``.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #210: TS-3607: Integrate ats_pagespeed into the automake...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the issue:
https://github.com/apache/trafficserver/pull/210
This would be a great to add pagespeed to the build. The patch would need to be updated against the master branch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3607: Integrate ats_pagespeed into ...
Posted by oschaaf <gi...@git.apache.org>.
Github user oschaaf commented on the pull request:
https://github.com/apache/trafficserver/pull/210#issuecomment-217549105
@zwoop done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---